[PATCH] D102251: Suppress Deferred Diagnostics in discarded statements.

[Erich Keane via Phabricator via cfe-commits]

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