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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits