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