rjmccall added inline comments.
================
Comment at: clang/include/clang/AST/StmtVisitor.h:146
+ RetTy VisitBin##NAME(PTR(BinaryOperator) S, ParamTys... P) {
\
+ DISPATCH(BinAssign, BinaryOperator);
\
}
----------------
erichkeane wrote:
> rjmccall wrote:
> > Comment needs updating, but more importantly, you're making all of these
> > fall back on `VisitBinAssign`, which is definitely not correct.
> @rjmccall : What would you think fits better? If we don't have something
> like this, VisitBinaryOperator is going to have to redirect to VisitBinAssign
> anyway. We could presumably keep VisitCompoundAssignOperator, but that seems
> like a naming difference.
`VisitBinAssign` is an existing method that is used specifically for the simple
assignment operator. The old delegation was that `VisitBinMulAssign` delegated
to `VisitCompoundAssignOperator`, which delegated to `VisitBinaryOperator`
(because CAO was a subclass of that), which delegated to `VisitExpr`. The
natural new delegation here would be for `VisitBinMulAssign` to just delegate
directly to `VisitBinaryOperator`. If it's still useful to have an
intermediate visitor method in the delegation chain for all the compound
assignment operators, that's fine, but it shouldn't be `VisitBinAssign`.
================
Comment at: clang/include/clang/Basic/LangOptions.h:394
+ return true;
+ }
+
----------------
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.
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
