[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added a comment. This needs to be commited https://reviews.llvm.org/D66487 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838 ___ cfe-commits mailing list

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

vitalybuka added a comment. Compilation fails with this patch http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/22936/steps/bootstrap%20clang/logs/stdio [ 8%] Built target LLVMMC /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:541:2: error: unannotated fall-through

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

This revision was automatically updated to reflect the committed changes. Closed by commit rL369414: [Attr] Support _attribute__ ((fallthrough)) (authored by Nathan-Huckleberry, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This LGTM, but you should hold off a bit to commit -- @rsmith, do you have any concerns with this approach? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry updated this revision to Diff 213435. Nathan-Huckleberry added a comment. - Remove 'maybe', remove boolean and fix other call to ParseSimpleDeclaration Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:1767 } return ParseSimpleDeclaration(Context, DeclEnd, attrs, true); Should this also be passed `DeclSpecStart`?

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added a comment. +1, looks good Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry updated this revision to Diff 213407. Nathan-Huckleberry added a comment. - Remove changes from accidentally formatted files Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838 Files:

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry updated this revision to Diff 213406. Nathan-Huckleberry added a comment. - Allow decl-specifier source location to propagate to decl parsing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry updated this revision to Diff 212198. Nathan-Huckleberry added a comment. - Fix test case spacing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838 Files:

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:233 +GNUAttributeLoc = Tok.getLocation(); +MaybeParseGNUAttributes(Attrs); +goto Retry; Since you know that tok is kw_attr, I think you can use 'ParseGNUAttributes'.

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added a comment. Thanks! Better and better:) Change in clang/test/Index/blocks.c seems not ideal, or is it okay? @aaron.ballman Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry updated this revision to Diff 212189. Nathan-Huckleberry added a comment. - Fix test, formatting and conditional check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838 Files:

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added inline comments. Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1228 + if (MacroName.empty()) { +if (!PreferClangAttr) + MacroName = "[[fallthrough]]"; This code is not tested? I think you can use tests from my older patch.

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:103 MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true); + if (!MaybeParseOpenCLUnrollHintAttribute(Attrs)) Spurious newline? Comment

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry updated this revision to Diff 212012. Nathan-Huckleberry added a comment. Herald added a subscriber: arphaman. - Rework attribute parsing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry updated this revision to Diff 212014. Nathan-Huckleberry added a comment. - Formatting fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838 Files: clang/include/clang/Basic/Attr.td

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

rsmith added a comment. In D64838#1602840 , @Nathan-Huckleberry wrote: > I agree that parsing according to attribute name/type is not a good solution. > > It sounds like we have narrowed it down to two choices: > Do we want to follow the gcc method of

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry added a comment. I agree that parsing according to attribute name/type is not a good solution. It sounds like we have narrowed it down to two choices: Do we want to follow the gcc method of parsing once and falling back if parsing fails? Do we want to parse attributes first

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added a comment. In D64838#1597771 , @xbolva00 wrote: > >> they parse the attributes first then attempt to parse a declaration; if > >> that fails, they fall back to parsing a statement > > Well, I don’t think this reparsing is ideal in

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added a comment. >> they parse the attributes first then attempt to parse a declaration; if >> that fails, they fall back to parsing a statement Well, I don’t think this reparsing is ideal in terms of compile time either. If we really care about attributes on implicit ints: I don’t

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry added a comment. In D64838#1593516 , @aaron.ballman wrote: > In D64838#1592520 , > @Nathan-Huckleberry wrote: > > > void foo() { > > __attribute__((address_space(0))) *x; > > *y; > >

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added a comment. In D64838#1592520 , @Nathan-Huckleberry wrote: > void foo() { > __attribute__((address_space(0))) *x; > *y; > } > > > If the attributes are parsed then function body looks like this to the parser: > > { >

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry added a comment. void foo() { __attribute__((address_space(0))) *x; *y; } If the attributes are parsed then the rest of the statement is identical to: { *x; //this one has attributes now *y; { The first line should be a valid declaration and the

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added a comment. In D64838#1592118 , @Nathan-Huckleberry wrote: > In D64838#1592111 , @aaron.ballman > wrote: > > > In D64838#1589770 , > >

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry added a comment. In D64838#1592111 , @aaron.ballman wrote: > In D64838#1589770 , > @Nathan-Huckleberry wrote: > > > The main problem that we have is that the `__attribute__` token always > >

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added a comment. In D64838#1589770 , @Nathan-Huckleberry wrote: > The main problem that we have is that the `__attribute__` token always causes > the parser to read the line as a declaration. Then the declaration parser > handles reading

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

efriedma added a comment. Yes, that seems reasonable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry added a comment. The main problem that we have is that the `__attribute__` token always causes the parser to read the line as a declaration. Then the declaration parser handles reading the attributes list. This case demonstrates the problem: void foo() {

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

rsmith added inline comments. Comment at: clang/lib/Parse/ParseTentative.cpp:2130 + ParsedAttributesWithRange attrs(AttrFactory); + ParseGNUAttributes(attrs, nullptr, nullptr); + if (attrs.size() <= 0) { It's not correct in general to call arbitrary parsing

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

efriedma added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:104 + + if (isNullStmtWithAttributes()) { +MaybeParseGNUAttributes(Attrs); If we're going to generally support statement attributes, it should be possible to apply them to non-null

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added inline comments. Comment at: clang/lib/Parse/ParseTentative.cpp:2146 +bool Parser::isNullStmtWithAttributes() { + RevertingTentativeParsingAction PA(*this); + return TryParseNullStmtWithAttributes() == TPResult::True; Is this “cheap” in terms of

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added a comment. Thanks, I think this is fine solution for now. Probably not ideal (@aaron.ballman mentioned the ideal solution - rewrite the parser), but “suboptimal” parser should not stop any progress in this area. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

xbolva00 added inline comments. Comment at: clang/lib/Parse/ParseTentative.cpp:2131 + ParseGNUAttributes(attrs, nullptr, nullptr); + if (attrs.size() <= 0) { +return TPResult::False; Negative size() ? Did you mean “== 0”? Not sure if “empty()” exists

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry added a comment. Revival of https://reviews.llvm.org/D63260 and https://reviews.llvm.org/D63299. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry updated this revision to Diff 210215. Nathan-Huckleberry added a comment. - Fixed formatting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64838/new/ https://reviews.llvm.org/D64838 Files: clang/include/clang/Basic/Attr.td

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

Nathan-Huckleberry created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fixed extraneous matches of non-NullStmt Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D64838 Files: clang/include/clang/Basic/Attr.td