[PATCH] D111568: [clang] Omit most AttributedStatements from the CFG

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

LGTM, thank you @thakis!



================
Comment at: clang/lib/Analysis/CFG.cpp:2407
+  return hasSpecificAttr<FallThroughAttr>(A->getAttrs()) &&
+         isa<NullStmt>(A->getSubStmt());
+}
----------------
hans wrote:
> thakis wrote:
> > hans wrote:
> > > Can fallthrough statements ever have children? If not, should it be an assert instead of a check here?
> > Good question. Attr.td says:
> > 
> > ```
> >   // The attribute only applies to a NullStmt, but we have special fix-it
> >   // behavior if applied to a case label.
> >   let Subjects = SubjectList<[NullStmt, SwitchCase], ErrorDiag,
> >                              "empty statements">;
> > ```
> > 
> > Which I suppose triggers for this:
> > 
> > ```
> >   switch (argc) {
> >     [[fallthrough]] case 4:
> >       break;
> >   }
> > ```
> > 
> > ```
> > foo.cc:6:7: error: 'fallthrough' attribute is only allowed on empty statements
> >     [[fallthrough]] case 4:
> >       ^             ~~~~
> > foo.cc:6:20: note: did you forget ';'?
> >     [[fallthrough]] case 4:
> > ```
> > 
> > But that doesn't seem to make it into the AST, according to -dump-ast. So I suppose it could be an assert as well. Want me to change this?
> > 
> > 
> Yes, I think an assert would make sense, otherwise the reader has to think about what would the code be doing for an AttributedStmt with non-null substmt.
I think it's fine to assert because we're required to reject fallthrough statements applying to something other than a null statement: https://eel.is/c++draft/dcl.attr.fallthrough#1.sentence-1


================
Comment at: clang/lib/Analysis/CFG.cpp:2418-2420
+  // also no children, and omit the others. None of the other current StmtAttrs
+  // have semantic meaning for the CFG.
+  if (isFallthroughStatement(A) && asc.alwaysAdd(*this, A)) {
----------------
thakis wrote:
> aaron.ballman wrote:
> > What about `OpenCLUnrollHintAttr`, `NoMergeAttr`, and `MustTailAttr`? These all have some semantic effect as statement attributes in terms of changing codegen, but perhaps they don't need modelling in the CFG?
> > 
> > (I'm trying to decide whether we may want to tablegen this functionality and so we might want something more general than `isFallthroughStatement()`.)
> Right, I think they all have no interesting effect on the CFG.
> 
> It's hard to predict the future, so I'd say let's wait and see until there are more StmtAttrs :)
SGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111568/new/

https://reviews.llvm.org/D111568