Page MenuHomePhabricator

Suppress Deferred Diagnostics in discarded statements.
ClosedPublic

Authored by erichkeane on May 11 2021, 9:35 AM.

Details

Summary

It doesn't really make sense to emit language specific diagnostics
in a discarded statement, and suppressing these diagnostics results in a
programming pattern that many users will feel is quite useful.

Basically, this makes sure we only emit errors from the 'true' side of a
'constexpr if'.

Diff Detail

Event Timeline

erichkeane requested review of this revision.May 11 2021, 9:35 AM
erichkeane created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 9:35 AM
ABataev added inline comments.May 11 2021, 9:54 AM
clang/include/clang/AST/Stmt.h
2082–2084

Do you really need Optional here? Just nullptr is not enough?

erichkeane added inline comments.May 11 2021, 9:58 AM
clang/include/clang/AST/Stmt.h
2082–2084

Yes for 2 reasons:
1- This is the existing interface, I didn't see reason to change it.

2- Optional::None and nullptr are two different 'cases'. None is returned here when this is not a constexpr-if. Nullptr is returned in cases where it _IS_ a constexpr-if, but the nondiscarded case is not present. For example:

if (foo) {} // Returns None.

if constexpr (false) {} // Returns nullptr.

if constexpr (false) else {} // Returns the 'else' compound statement.
This revision is now accepted and ready to land.May 11 2021, 9:59 AM

LG

Thanks! I'm going to give @yaxunl a day to take a look that this, particularly since the only test I could come up with was CUDA based.

LG

Thanks! I'm going to give @yaxunl a day to take a look that this, particularly since the only test I could come up with was CUDA based.

Yep!

tra accepted this revision.May 11 2021, 10:38 AM
tra added a subscriber: tra.

LGTM for CUDA. This matches the intent of deferred diags -- we only emit them if we get to generate the code for the sources that triggered them, so they should not show up for the false constexpr branches.

yaxunl accepted this revision.May 11 2021, 1:18 PM

LGTM. Thanks.

rsmith added a subscriber: rsmith.May 11 2021, 5:18 PM

(No blocking concerns here.)

clang/lib/AST/Stmt.cpp
1000–1003

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>.

clang/lib/Sema/Sema.cpp
1545

I wonder if there are other UsedDeclVisitors 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.

erichkeane added inline comments.May 12 2021, 6:11 AM
clang/lib/AST/Stmt.cpp
1000–1003

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

clang/lib/Sema/Sema.cpp
1545

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.

Moved IfStmt/discarded case logic to ExprEvaluatorBase per @rsmith 's suggestion. This is the 'highest' in the tree that has a Sema/ASTContext reference, so anything 'higher' in the tree would be higher touch. @rsmith: WDYT?

rsmith accepted this revision.May 12 2021, 11:08 AM
rsmith added inline comments.
clang/include/clang/AST/EvaluatedExprVisitor.h
37

Maybe shouldVisitDiscardedStmt would be more obvious, since "discarded statement" is the standard terminology.

erichkeane added inline comments.May 12 2021, 12:08 PM
clang/include/clang/AST/EvaluatedExprVisitor.h
37

Yep, agreed. I'll do that as a part of the commit.

This revision was automatically updated to reflect the committed changes.