haberman added inline comments.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:588
+
+ const CallExpr *CE = dyn_cast<CallExpr>(Ex->IgnoreUnlessSpelledInSource());
+
----------------
rsmith wrote:
> `IgnoreUnlessSpelledInSource` is a syntactic check that's only really
> intended for tooling use cases; I think we want something a bit more semantic
> here, so `IgnoreImplicitAsWritten` would be more appropriate.
>
> I think it would be reasonable to also skip "parentheses" here (which we
> treat as also including things like C's `_Generic`). Would
> `Ex->IgnoreImplicitAsWritten()->IgnoreParens()` work?
>
> If we're going to skip elidable copy construction of the result here (which I
> think we should), should we also reflect that in the AST? Perhaps we should
> strip the return value down to being just the call expression? I'm thinking
> in particular of things like building in C++14 or before with
> `-fno-elide-constructors`, where code generation for a by-value return of a
> class object will synthesize a local temporary to hold the result, with a
> final destination copy emitted after the call. (Testcase: `struct A { A(const
> A&); }; A f(); A g() { [[clang::musttail]] return f(); }` with
> `-fno-elide-constructors`.)
`IgnoreImplicitAsWritten()` doesn't skip `ExprWithCleanups`, and per your
previous comment I was trying to find a `CallExpr` before doing the check
prohibiting `ExprWithCleanups` with side effects.
I could write some custom ignore logic using `clang::IgnoreExprNodes()`
directly.
> If we're going to skip elidable copy construction of the result here (which I
> think we should)
To clarify, are you suggesting that we allow `musttail` through elidable copy
constructors on the return value, even if `-fno-elide-constructors` is set? ie.
we consider that `musttail` overrides the `-fno-elide-constructors` option on
the command line?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+ for (const auto *A : Attrs) {
+ if (A->getKind() == attr::MustTail) {
+ if (!checkMustTailAttr(SubStmt, *A)) {
+ return SubStmt;
+ }
+ setFunctionHasMustTail();
+ }
----------------
aaron.ballman wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > That is where I had originally put it, but that didn't work for templates.
> > The semantic checks can only be performed at instantiation time.
> > `ActOnAttributedStmt` seems to be the right hook point where I can evaluate
> > the semantic checks for both template and non-template functions (with
> > template functions getting checked at instantiation time).
> I disagree that `ActOnAttributedStmt()` is the correct place for this
> checking -- template checking should occur when the template is instantiated,
> same as happens for declaration attributes. I'd like to see this
> functionality moved to SemaStmtAttr.cpp. Keeping the attribute logic together
> and following the same patterns is what allows us to tablegenerate more of
> the attribute logic. Statement attributes are just starting to get more such
> automation.
I tried commenting out this code and adding the following code into
`handleMustTailAttr()` in `SemaStmtAttr.cpp`:
```
if (!S.checkMustTailAttr(St, MTA))
return nullptr;
```
This caused my test cases related to templates to fail. It also seemed to break
test cases related to `JumpDiagnostics`. My interpretation of this is that
`handleMustTailAttr()` is called during parsing only, and cannot catch errors
at template instantiation time or that require a more complete AST.
What am I missing? Where in SemaStmtAttr.cpp are you suggesting that I put this
check?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99517/new/
https://reviews.llvm.org/D99517
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
