aaron.ballman added inline comments.
================
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();
+ }
----------------
haberman wrote:
> aaron.ballman wrote:
> > haberman wrote:
> > > haberman wrote:
> > > > 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?
> > > Scratch the part about `JumpDiagnostics`, that was me failing to call
> > > `S.setFunctionHasMustTail()`. I added that and now the `JumpDiagnostics`
> > > tests pass.
> > >
> > > But the template test cases still fail, and I can't find any hook point
> > > in `SemaStmtAttr.cpp` that will let me evaluate these checks at template
> > > instantiation time.
> > I think there's a bit of an architectural mixup, but I'm curious if @rsmith
> > agrees before anyone starts doing work to make changes.
> >
> > When transforming declarations, `RebuildWhatever()` calls the
> > `ActOnWhatever()` function which calls `ProcessDeclAttributeList()` so that
> > attributes are processed. `RebuildAttributedStmt()` similarly calls
> > `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` doesn't call
> > `ProcessStmtAttributes()` -- the logic is reversed so that
> > `ProcessStmtAttributes()` is what calls `ActOnAttributedStmt()`.
> >
> > I think the correct answer is to switch the logic so that
> > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the template
> > logic should automatically work.
> > I think the correct answer is to switch the logic so that
> > ActOnAttributedStmt() calls ProcessStmtAttributes()
>
> I think this would require `ProcessStmtAttributes()` to be split into two
> separate functions. Currently that function is doing two separate things:
>
> 1. Translation of `ParsedAttr` into various subclasses of `Attr`.
> 2. Validation that the attribute is semantically valid.
>
> The function signature for `ActOnAttributedStmt()` uses `Attr` (not
> `ParsedAttr`), so (1) must happen during the parse, before
> `ActOnAttributedStmt()` is called. But (2) must be deferred until template
> instantiation time for some cases, like `musttail`.
I don't think the signature for `ActOnAttributedStmt()` is correct to use
`Attr` instead of `ParsedAttr`. I think it should be `StmtResult
ActOnAttributedStmt(const ParsedAttributesViewWithRange &AttrList, Stmt
*SubStmt);` -- this likely requires a fair bit of surgery to make work though,
which is why I'd like to hear from @rsmith if he agrees with the approach. In
the meantime, I'll play around with this idea locally in more depth.
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
