erichkeane added inline comments.
================ Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + ---------------- rjmccall wrote: > erichkeane wrote: > > rjmccall wrote: > > > 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. > > Right, I get the intent, and I completely agree with that. My point was > > EVERY Expr that is affected by a #pragma should store it. Though, after > > looking at your Macro concern above, I'm less compelled. > > > > I guess was suggesting that the logic for "requiresTrailingStorage" should > > just be "modified by a pragma" instead of "FPOptions != The global setting". > Well, "modified by a pragma" still wouldn't make the AST agnostic to global > settings, since the pragmas don't override everything in FPOptions at once. > But I agree that would achieve the most important goal, which is to stop > inflating the AST when pragmas *aren't* in effect, which is approximately > 100% of the time. In order to do that, though, we'll need to start tracking > pragmas, which we should do but which can wait for a follow-up patch. In the > meantime, I don't think you're ever going to get the interfaces right for > queries like `BinaryOperator::getFPOptions` unless you actually stop relying > on the fact that you're unconditionally storing `FPOptions`. You need to > passing around ASTContexts for that. That's why I'm suggesting using an > exact match with the global settings as a simple thing you can easily check > without modifying what data you collect in `FPOptions`. That sounds like a good plan to me. Thanks for entertaining my conversation/questions. 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
