erichkeane added inline comments.
================ Comment at: clang/lib/AST/Stmt.cpp:1000-1003 + if (Optional<Stmt *> Result = + const_cast<IfStmt *>(this)->getNondiscardedCase(Ctx)) + return *Result; + return None; ---------------- rsmith wrote: > Can this simply be > ``` > return const_cast<IfStmt *>(this)->getNondiscardedCase(Ctx)); > ``` > ? I would expect that if `T` implicitly converts to `U` then `Optional<T>` > implicitly converts to `Optional<U>`. that does not seem to be the case unfortunately. `llvm::Optional` doesn't have any such conversion functions. Its only constructors and or `operator=` just take `Optional`. It is missing most of the constructors here: https://en.cppreference.com/w/cpp/utility/optional/optional ================ Comment at: clang/lib/Sema/Sema.cpp:1545 class DeferredDiagnosticsEmitter : public UsedDeclVisitor<DeferredDiagnosticsEmitter> { public: ---------------- rsmith wrote: > I wonder if there are other `UsedDeclVisitor`s that would want this behavior, > largely because of [basic.def.odr]p10: "Every program shall contain exactly > one definition of every non-inline function or variable that is odr-used in > that program outside of a discarded statement (8.5.2)". It seems worth > checking, and maybe moving this functionality to an optional behavior in the > base class, but I don't want to hold up this patch on that. I've actually implemented this 2x (another time in a different class, that inherits from a different visitor, I believe `CallGraph`) in our downstream. The issue in many cases is that we lack access to `ASTContext` in those. HOWEVER, I note that `EvaluatedExprVisitorBase` actually has access to an `ASTContext`, so I can put that in there controlled by an inherited function (like we do with some of the other visitors). If it ends up being low-touch enough, I'll move it as an opt-in behavior for this patch this morning. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102251/new/ https://reviews.llvm.org/D102251 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
