rjmccall added inline comments.
================ Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + ---------------- rjmccall wrote: > erichkeane wrote: > > rjmccall wrote: > > > The problem with having both functions that take `ASTContext`s and > > > functions that don't is that it's easy to mix them, so they either need > > > to have the same behavior (in which case it's pointless to have an > > > overload that takes the `ASTContext`) or you're making something really > > > error-prone. > > > > > > I would feel a lot more confident that you were designing and using these > > > APIs correctly if you actually took advantage of the ability to not store > > > trailing FPOptions in some case, like when they match the global settings > > > in the ASTContext. That way you'll actually be verifying that everything > > > behaves correctly if nodes don't store FPOptions. If you do that, I > > > think you'll see my point about not having all these easily-confusable > > > functions that do or do not take `ASTContext`s.. > > I think I disagree with @rjmccall that these requiresTrailingStorage should > > be here at all. I think we should store in the AST ANY programmer opinion, > > even if they match the global setting. It seems to me that this would be > > more tolerant of any global-setting rewrites that modules/et-al introduce, > > as well as make the AST Print consistent. Always storing FPOptions when > > the user has explicitly overriding it also better captures the programmer's > > intent. > I covered this elsewhere in the review. If you want to have that tolerance — > and I think you should — then expressions should store (and Sema should > track) the active pragma state, which can be most easily expressed as a pair > of an FPOptions and a mask to apply to the global FPOptions. When you enter > a pragma, you clear the relevant bits from the global FPOptions mask. > > But the whole point of putting this stuff in trailing storage is so that you > can make FPOptions as big as you need without actually inflating the AST size > for a million nodes that don't care in the slightest about FPOptions. > But the whole point of putting this stuff in trailing storage is so that you > can make FPOptions as big as you need without actually inflating the AST size > for a million nodes that don't care in the slightest about FPOptions. I meant to say: for a million nodes that don't care in the slightest about FPOptions, as well as for a million more nodes that aren't using pragma overrides. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/ https://reviews.llvm.org/D76384 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
