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

[Dávid Bolvanský via Phabricator via cfe-commits]

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

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

[Vitaly Buka via Phabricator via cfe-commits]

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 between switch labels [-Werror,-Wimplicit-fallthrough]
  default:
  ^
  /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:541:2: note: 
insert '__attribute__((fallthrough));' to silence this warning
  default:
  ^
  __attribute__((fallthrough)); 
  /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:541:2: note: 
insert 'break;' to avoid fall-through
  default:
  ^
  break; 
  /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:737:2: error: 
unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  default:
  ^
  /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:737:2: note: 
insert '__attribute__((fallthrough));' to silence this warning
  default:
  ^
  __attribute__((fallthrough)); 
  /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:737:2: note: 
insert 'break;' to avoid fall-through
  default:
  ^
  break; 
  /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:1639:3: error: 
unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  default:/* things that break a sequence */
  ^
  /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:1639:3: note: 
insert '__attribute__((fallthrough));' to silence this warning
  default:/* things that break a sequence */
  ^
  __attribute__((fallthrough)); 
  /b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:1639:3: note: 
insert 'break;' to avoid fall-through
  default:/* things that break a sequence */
  ^
  break; 
  3 errors generated.
  lib/Support/CMakeFiles/LLVMSupport.dir/build.make:2558: recipe for target 
'lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o' failed
  make[3]: *** [lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o] Error 1
  CMakeFiles/Makefile2:1206: recipe for target 
'lib/Support/CMakeFiles/LLVMSupport.dir/all' failed
  make[2]: *** [lib/Support/CMakeFiles/LLVMSupport.dir/all] Error 2
  CMakeFiles/Makefile2:22547: recipe for target 
'tools/gold/CMakeFiles/LLVMgold.dir/rule' failed
  make[1]: *** [tools/gold/CMakeFiles/LLVMgold.dir/rule] Error 2
  Makefile:6905: recipe for target 'LLVMgold' failed
  make: *** [LLVMgold] Error 2
  + echo @@@STEP_FAILURE@@@
  + check_64bit 1 sanitizer
  + CONDITION=1
  + SANITIZER=sanitizer
  @@@STEP_FAILURE@@@
  + '[' 1 == 1 ']'
  + echo @@@BUILD_STEP 64-bit check-sanitizer@@@
  
  
  

  started: Wed Aug 21 01:01:43 2019
  ended: Wed Aug 21 01:17:17 2019
  duration: 15 mins, 34 secs


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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:
  https://reviews.llvm.org/D64838?vs=213435=216191#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64838

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Parse/Parser.h
  cfe/trunk/lib/Parse/ParseDecl.cpp
  cfe/trunk/lib/Parse/ParseStmt.cpp
  cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
  cfe/trunk/test/Sema/fallthrough-attr.c
  cfe/trunk/test/SemaCXX/switch-implicit-fallthrough.cpp
  cfe/trunk/test/SemaCXX/warn-unused-label-error.cpp

Index: cfe/trunk/test/SemaCXX/warn-unused-label-error.cpp
===
--- cfe/trunk/test/SemaCXX/warn-unused-label-error.cpp
+++ cfe/trunk/test/SemaCXX/warn-unused-label-error.cpp
@@ -18,9 +18,9 @@
   }
 
   void h() {
-D: // expected-warning {{unused label 'D'}}
-  #pragma weak unused_local_static
-  __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
-  ;
+  D:
+#pragma weak unused_local_static
+__attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
+;
   }
 }
Index: cfe/trunk/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- cfe/trunk/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ cfe/trunk/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: cfe/trunk/test/Sema/fallthrough-attr.c
===
--- cfe/trunk/test/Sema/fallthrough-attr.c
+++ cfe/trunk/test/Sema/fallthrough-attr.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=gnu99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DC2X -verify -Wimplicit-fallthrough %s
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+  case 1:
+#if defined(C2X)
+// expected-warning@-2{{unannotated fall-through between switch labels}} expected-note@-2{{insert '[[fallthrough]];' to silence this warning}} expected-note@-2{{insert 'break;' to avoid fall-through}}
+#else
+// expected-warning@-4{{unannotated fall-through between switch labels}} expected-note@-4{{insert '__attribute__((fallthrough));' to silence this warning}} expected-note@-4{{insert 'break;' to avoid fall-through}}
+#endif
+n++;
+__attribute__((fallthrough));
+  case 2:
+n++;
+break;
+  }
+  return n;
+}
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1215,7 +1215,7 @@
 tok::r_square, tok::r_square
   };
 
-  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17;
+  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17 && !PP.getLangOpts().C2x;
 
   StringRef MacroName;
   if (PreferClangAttr)
@@ -1224,24 +1224,19 @@
 MacroName = PP.getLastMacroWithSpelling(Loc, FallthroughTokens);
   if (MacroName.empty() && !PreferClangAttr)
 MacroName = PP.getLastMacroWithSpelling(Loc, ClangFallthroughTokens);
-  if (MacroName.empty())
-MacroName = PreferClangAttr ? "[[clang::fallthrough]]" : "[[fallthrough]]";
+  if (MacroName.empty()) {
+if (!PreferClangAttr)
+  MacroName = "[[fallthrough]]";
+else if (PP.getLangOpts().CPlusPlus)
+  MacroName = "[[clang::fallthrough]]";
+else
+  MacroName = "__attribute__((fallthrough))";
+  }
   return MacroName;
 }
 
 static void DiagnoseSwitchLabelsFallthrough(Sema , AnalysisDeclContext ,
 bool PerFunction) {
-  // Only perform this analysis when using [[]] attributes. There is no good
-  // workflow for this warning when not using C++11. There is no good way to
-  // silence the warning (no attribute is available) unless we are using
-  // [[]] attributes. One could use pragmas to silence the warning, but as a
-  // general solution that is gross and not in the spirit of this warning.
-  //
-  // NOTE: This an intermediate solution. There are on-going discussions on
-  // how to properly support this warning outside of C++11 with an annotation.
-  if (!AC.getASTContext().getLangOpts().DoubleSquareBracketAttributes)
-  

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

[Aaron Ballman via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D64838/new/

https://reviews.llvm.org/D64838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D64838

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp
  clang/test/SemaCXX/warn-unused-label-error.cpp

Index: clang/test/SemaCXX/warn-unused-label-error.cpp
===
--- clang/test/SemaCXX/warn-unused-label-error.cpp
+++ clang/test/SemaCXX/warn-unused-label-error.cpp
@@ -18,9 +18,9 @@
   }
 
   void h() {
-D: // expected-warning {{unused label 'D'}}
-  #pragma weak unused_local_static
-  __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
-  ;
+  D:
+#pragma weak unused_local_static
+__attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
+;
   }
 }
Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=gnu99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DC2X -verify -Wimplicit-fallthrough %s
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+  case 1:
+#if defined(C2X)
+// expected-warning@-2{{unannotated fall-through between switch labels}} expected-note@-2{{insert '[[fallthrough]];' to silence this warning}} expected-note@-2{{insert 'break;' to avoid fall-through}}
+#else
+// expected-warning@-4{{unannotated fall-through between switch labels}} expected-note@-4{{insert '__attribute__((fallthrough));' to silence this warning}} expected-note@-4{{insert 'break;' to avoid fall-through}}
+#endif
+n++;
+__attribute__((fallthrough));
+  case 2:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1215,7 +1215,7 @@
 tok::r_square, tok::r_square
   };
 
-  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17;
+  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17 && !PP.getLangOpts().C2x;
 
   StringRef MacroName;
   if (PreferClangAttr)
@@ -1224,24 +1224,19 @@
 MacroName = PP.getLastMacroWithSpelling(Loc, FallthroughTokens);
   if (MacroName.empty() && !PreferClangAttr)
 MacroName = PP.getLastMacroWithSpelling(Loc, ClangFallthroughTokens);
-  if (MacroName.empty())
-MacroName = PreferClangAttr ? "[[clang::fallthrough]]" : "[[fallthrough]]";
+  if (MacroName.empty()) {
+if (!PreferClangAttr)
+  MacroName = "[[fallthrough]]";
+else if (PP.getLangOpts().CPlusPlus)
+  MacroName = "[[clang::fallthrough]]";
+else
+  MacroName = "__attribute__((fallthrough))";
+  }
   return MacroName;
 }
 
 static void DiagnoseSwitchLabelsFallthrough(Sema , AnalysisDeclContext ,
 bool PerFunction) {
-  // Only perform this analysis when using [[]] attributes. There is no good
-  // workflow for this warning when not using C++11. There is no good way to
-  // silence the warning (no attribute is available) unless we are using
-  // [[]] attributes. One could use pragmas to silence the warning, but as a
-  // general solution that is gross and not in the spirit of this warning.
-  //
-  // NOTE: This an intermediate solution. There are on-going discussions on
-  // how to properly support this warning outside of C++11 with an annotation.
-  if (!AC.getASTContext().getLangOpts().DoubleSquareBracketAttributes)
-return;
-
   FallthroughMapper FM(S);
   FM.TraverseStmt(AC.getBody());
 
@@ -1281,25 +1276,24 @@
   SourceLocation L = Label->getBeginLoc();
   if (L.isMacroID())
 continue;
-  if (S.getLangOpts().CPlusPlus11) {
-const Stmt *Term = 

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

[Aaron Ballman via Phabricator via cfe-commits]

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`?



Comment at: clang/lib/Parse/ParseStmt.cpp:233
+GNUAttributeLoc = Tok.getLocation();
+MaybeParseGNUAttributes(Attrs);
+goto Retry;

xbolva00 wrote:
> Since you know that tok is kw_attr, I think you can use 'ParseGNUAttributes'.
Agreed, you don't need to use the Maybe check here.



Comment at: clang/lib/Parse/ParseStmt.cpp:156
   StmtResult Res;
+  bool SeenGNUAttributes = false;
+  SourceLocation GNUAttributeLoc;

I think you can use `GNUAttributeLoc.isValid()` instead of using the extra 
local variable.



Comment at: clang/test/SemaCXX/warn-unused-label-error.cpp:23
+#pragma weak unused_local_static
+__attribute__((unused)) // expected-error {{'unused' attribute cannot be 
applied to a statement}}
+;

This change in diagnostics makes me very happy!


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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp
  clang/test/SemaCXX/warn-unused-label-error.cpp

Index: clang/test/SemaCXX/warn-unused-label-error.cpp
===
--- clang/test/SemaCXX/warn-unused-label-error.cpp
+++ clang/test/SemaCXX/warn-unused-label-error.cpp
@@ -18,9 +18,9 @@
   }
 
   void h() {
-D: // expected-warning {{unused label 'D'}}
-  #pragma weak unused_local_static
-  __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
-  ;
+  D:
+#pragma weak unused_local_static
+__attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
+;
   }
 }
Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=gnu99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DC2X -verify -Wimplicit-fallthrough %s
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+  case 1:
+#if defined(C2X)
+// expected-warning@-2{{unannotated fall-through between switch labels}} expected-note@-2{{insert '[[fallthrough]];' to silence this warning}} expected-note@-2{{insert 'break;' to avoid fall-through}}
+#else
+// expected-warning@-4{{unannotated fall-through between switch labels}} expected-note@-4{{insert '__attribute__((fallthrough));' to silence this warning}} expected-note@-4{{insert 'break;' to avoid fall-through}}
+#endif
+n++;
+__attribute__((fallthrough));
+  case 2:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1215,7 +1215,7 @@
 tok::r_square, tok::r_square
   };
 
-  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17;
+  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17 && !PP.getLangOpts().C2x;
 
   StringRef MacroName;
   if (PreferClangAttr)
@@ -1224,24 +1224,19 @@
 MacroName = PP.getLastMacroWithSpelling(Loc, FallthroughTokens);
   if (MacroName.empty() && !PreferClangAttr)
 MacroName = PP.getLastMacroWithSpelling(Loc, ClangFallthroughTokens);
-  if (MacroName.empty())
-MacroName = PreferClangAttr ? "[[clang::fallthrough]]" : "[[fallthrough]]";
+  if (MacroName.empty()) {
+if (!PreferClangAttr)
+  MacroName = "[[fallthrough]]";
+else if (PP.getLangOpts().CPlusPlus)
+  MacroName = "[[clang::fallthrough]]";
+else
+  MacroName = "__attribute__((fallthrough))";
+  }
   return MacroName;
 }
 
 static void DiagnoseSwitchLabelsFallthrough(Sema , AnalysisDeclContext ,
 bool PerFunction) {
-  // Only perform this analysis when using [[]] attributes. There is no good
-  // workflow for this warning when not using C++11. There is no good way to
-  // silence the warning (no attribute is available) unless we are using
-  // [[]] attributes. One could use pragmas to silence the warning, but as a
-  // general solution that is gross and not in the spirit of this warning.
-  //
-  // NOTE: This an intermediate solution. There are on-going discussions on
-  // how to properly support this warning outside of C++11 with an annotation.
-  if (!AC.getASTContext().getLangOpts().DoubleSquareBracketAttributes)
-return;
-
   FallthroughMapper FM(S);
   FM.TraverseStmt(AC.getBody());
 
@@ -1281,25 +1276,24 @@
   SourceLocation L = Label->getBeginLoc();
   if (L.isMacroID())
 continue;
-  if (S.getLangOpts().CPlusPlus11) {
-const Stmt *Term = B->getTerminatorStmt();
-  

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D64838

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Index/blocks.c
  clang/test/Index/load-exprs.c
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp
  clang/test/SemaCXX/warn-unused-label-error.cpp

Index: clang/test/SemaCXX/warn-unused-label-error.cpp
===
--- clang/test/SemaCXX/warn-unused-label-error.cpp
+++ clang/test/SemaCXX/warn-unused-label-error.cpp
@@ -18,9 +18,9 @@
   }
 
   void h() {
-D: // expected-warning {{unused label 'D'}}
-  #pragma weak unused_local_static
-  __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
-  ;
+  D:
+#pragma weak unused_local_static
+__attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
+;
   }
 }
Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=gnu99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DC2X -verify -Wimplicit-fallthrough %s
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+  case 1:
+#if defined(C2X)
+// expected-warning@-2{{unannotated fall-through between switch labels}} expected-note@-2{{insert '[[fallthrough]];' to silence this warning}} expected-note@-2{{insert 'break;' to avoid fall-through}}
+#else
+// expected-warning@-4{{unannotated fall-through between switch labels}} expected-note@-4{{insert '__attribute__((fallthrough));' to silence this warning}} expected-note@-4{{insert 'break;' to avoid fall-through}}
+#endif
+n++;
+__attribute__((fallthrough));
+  case 2:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Index/load-exprs.c
===
--- clang/test/Index/load-exprs.c
+++ clang/test/Index/load-exprs.c
@@ -78,4 +78,3 @@
 // CHECK: load-exprs.c:31:32: MemberRef=array:24:12 Extent=[31:32 - 31:37]
 // CHECK: load-exprs.c:31:38: DeclRefExpr=StartIndex:27:8 Extent=[31:38 - 31:48]
 // CHECK: load-exprs.c:31:50: MemberRef=b:2:19 Extent=[31:50 - 31:51]
-
Index: clang/test/Index/blocks.c
===
--- clang/test/Index/blocks.c
+++ clang/test/Index/blocks.c
@@ -31,4 +31,3 @@
 // CHECK: blocks.c:9:54: DeclRefExpr=i:8:11 Extent=[9:54 - 9:55]
 // CHECK: blocks.c:9:59: UnaryOperator= Extent=[9:59 - 9:64]
 // CHECK: blocks.c:9:60: DeclRefExpr=_foo:7:21 Extent=[9:60 - 9:64]
-
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1215,7 +1215,7 @@
 tok::r_square, tok::r_square
   };
 
-  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17;
+  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17 && !PP.getLangOpts().C2x;
 
   StringRef MacroName;
   if (PreferClangAttr)
@@ -1224,24 +1224,19 @@
 MacroName = PP.getLastMacroWithSpelling(Loc, FallthroughTokens);
   if (MacroName.empty() && !PreferClangAttr)
 MacroName = PP.getLastMacroWithSpelling(Loc, ClangFallthroughTokens);
-  if (MacroName.empty())
-MacroName = PreferClangAttr ? "[[clang::fallthrough]]" : "[[fallthrough]]";
+  if (MacroName.empty()) {
+if (!PreferClangAttr)
+  MacroName = "[[fallthrough]]";
+else if (PP.getLangOpts().CPlusPlus)
+  MacroName = "[[clang::fallthrough]]";
+else
+  MacroName = "__attribute__((fallthrough))";
+  }
   return MacroName;
 }
 
 static void DiagnoseSwitchLabelsFallthrough(Sema , AnalysisDeclContext ,
 bool PerFunction) {
-  // Only perform this analysis 

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Attr.td
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Index/blocks.c
  clang/test/Index/load-exprs.c
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp
  clang/test/SemaCXX/warn-unused-label-error.cpp

Index: clang/test/SemaCXX/warn-unused-label-error.cpp
===
--- clang/test/SemaCXX/warn-unused-label-error.cpp
+++ clang/test/SemaCXX/warn-unused-label-error.cpp
@@ -18,9 +18,9 @@
   }
 
   void h() {
-D: // expected-warning {{unused label 'D'}}
-  #pragma weak unused_local_static
-  __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
-  ;
+  D:
+#pragma weak unused_local_static
+__attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
+;
   }
 }
Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=gnu99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DC2X -verify -Wimplicit-fallthrough %s
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+  case 1:
+#if defined(C2X)
+// expected-warning@-2{{unannotated fall-through between switch labels}} expected-note@-2{{insert '[[fallthrough]];' to silence this warning}} expected-note@-2{{insert 'break;' to avoid fall-through}}
+#else
+// expected-warning@-4{{unannotated fall-through between switch labels}} expected-note@-4{{insert '__attribute__((fallthrough));' to silence this warning}} expected-note@-4{{insert 'break;' to avoid fall-through}}
+#endif
+n++;
+__attribute__((fallthrough));
+  case 2:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Index/load-exprs.c
===
--- clang/test/Index/load-exprs.c
+++ clang/test/Index/load-exprs.c
@@ -52,7 +52,7 @@
 // CHECK: load-exprs.c:7:23: DeclRefExpr=x:6:12 Extent=[7:23 - 7:24]
 // CHECK: load-exprs.c:10:5: FunctionDecl=test_blocks:10:5 (Definition) Extent=[10:1 - 21:2]
 // CHECK: load-exprs.c:10:21: ParmDecl=x:10:21 (Definition) Extent=[10:17 - 10:22]
-// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:3 - 11:20]
+// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:11 - 11:20]
 // CHECK: load-exprs.c:11:19: DeclRefExpr=x:10:21 Extent=[11:19 - 11:20]
 // CHECK: load-exprs.c:12:3: CallExpr= Extent=[12:3 - 19:7]
 // CHECK: load-exprs.c:13:17: VarDecl=z:13:17 (Definition) Extent=[13:6 - 13:22]
@@ -78,4 +78,3 @@
 // CHECK: load-exprs.c:31:32: MemberRef=array:24:12 Extent=[31:32 - 31:37]
 // CHECK: load-exprs.c:31:38: DeclRefExpr=StartIndex:27:8 Extent=[31:38 - 31:48]
 // CHECK: load-exprs.c:31:50: MemberRef=b:2:19 Extent=[31:50 - 31:51]
-
Index: clang/test/Index/blocks.c
===
--- clang/test/Index/blocks.c
+++ clang/test/Index/blocks.c
@@ -14,7 +14,7 @@
 // CHECK: blocks.c:7:3: DeclStmt= Extent=[7:3 - 7:26]
 // CHECK: blocks.c:7:21: VarDecl=_foo:7:21 (Definition) Extent=[7:3 - 7:25]
 // CHECK: blocks.c:7:17: TypeRef=struct foo:4:8 Extent=[7:17 - 7:20]
-// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:3 - 8:16]
+// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:11 - 8:16]
 // CHECK: blocks.c:8:15: IntegerLiteral= Extent=[8:15 - 8:16]
 // CHECK: blocks.c:9:3: CallExpr= Extent=[9:3 - 9:65]
 // CHECK: blocks.c:9:3: BlockExpr= Extent=[9:3 - 9:58]
@@ -31,4 +31,3 @@
 // CHECK: blocks.c:9:54: DeclRefExpr=i:8:11 Extent=[9:54 - 9:55]
 // CHECK: blocks.c:9:59: UnaryOperator= Extent=[9:59 - 9:64]
 // CHECK: blocks.c:9:60: DeclRefExpr=_foo:7:21 Extent=[9:60 - 9:64]
-
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- 

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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


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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Attr.td
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Index/blocks.c
  clang/test/Index/load-exprs.c
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp
  clang/test/SemaCXX/warn-unused-label-error.cpp

Index: clang/test/SemaCXX/warn-unused-label-error.cpp
===
--- clang/test/SemaCXX/warn-unused-label-error.cpp
+++ clang/test/SemaCXX/warn-unused-label-error.cpp
@@ -18,9 +18,9 @@
   }
 
   void h() {
-D: // expected-warning {{unused label 'D'}}
-  #pragma weak unused_local_static
-  __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
-  ;
+  D:
+#pragma weak unused_local_static
+__attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
+;
   }
 }
Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=gnu99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DC2X -verify -Wimplicit-fallthrough %s
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+  case 1:
+#if defined(C2X)
+// expected-warning@-2{{unannotated fall-through between switch labels}} expected-note@-2{{insert '[[fallthrough]];' to silence this warning}} expected-note@-2{{insert 'break;' to avoid fall-through}}
+#else
+// expected-warning@-4{{unannotated fall-through between switch labels}} expected-note@-4{{insert '__attribute__ ((fallthrough));' to silence this warning}} expected-note@-4{{insert 'break;' to avoid fall-through}}
+#endif
+n++;
+__attribute__((fallthrough));
+  case 2:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Index/load-exprs.c
===
--- clang/test/Index/load-exprs.c
+++ clang/test/Index/load-exprs.c
@@ -52,7 +52,7 @@
 // CHECK: load-exprs.c:7:23: DeclRefExpr=x:6:12 Extent=[7:23 - 7:24]
 // CHECK: load-exprs.c:10:5: FunctionDecl=test_blocks:10:5 (Definition) Extent=[10:1 - 21:2]
 // CHECK: load-exprs.c:10:21: ParmDecl=x:10:21 (Definition) Extent=[10:17 - 10:22]
-// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:3 - 11:20]
+// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:11 - 11:20]
 // CHECK: load-exprs.c:11:19: DeclRefExpr=x:10:21 Extent=[11:19 - 11:20]
 // CHECK: load-exprs.c:12:3: CallExpr= Extent=[12:3 - 19:7]
 // CHECK: load-exprs.c:13:17: VarDecl=z:13:17 (Definition) Extent=[13:6 - 13:22]
@@ -78,4 +78,3 @@
 // CHECK: load-exprs.c:31:32: MemberRef=array:24:12 Extent=[31:32 - 31:37]
 // CHECK: load-exprs.c:31:38: DeclRefExpr=StartIndex:27:8 Extent=[31:38 - 31:48]
 // CHECK: load-exprs.c:31:50: MemberRef=b:2:19 Extent=[31:50 - 31:51]
-
Index: clang/test/Index/blocks.c
===
--- clang/test/Index/blocks.c
+++ clang/test/Index/blocks.c
@@ -14,7 +14,7 @@
 // CHECK: blocks.c:7:3: DeclStmt= Extent=[7:3 - 7:26]
 // CHECK: blocks.c:7:21: VarDecl=_foo:7:21 (Definition) Extent=[7:3 - 7:25]
 // CHECK: blocks.c:7:17: TypeRef=struct foo:4:8 Extent=[7:17 - 7:20]
-// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:3 - 8:16]
+// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:11 - 8:16]
 // CHECK: blocks.c:8:15: IntegerLiteral= Extent=[8:15 - 8:16]
 // CHECK: blocks.c:9:3: CallExpr= Extent=[9:3 - 9:65]
 // CHECK: blocks.c:9:3: BlockExpr= Extent=[9:3 - 9:58]
@@ -31,4 +31,3 @@
 // CHECK: blocks.c:9:54: DeclRefExpr=i:8:11 Extent=[9:54 - 9:55]
 // CHECK: blocks.c:9:59: UnaryOperator= Extent=[9:59 - 9:64]
 // CHECK: blocks.c:9:60: DeclRefExpr=_foo:7:21 Extent=[9:60 - 9:64]
-
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- 

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseStmt.cpp:103
   MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
+
   if (!MaybeParseOpenCLUnrollHintAttribute(Attrs))

Spurious newline?



Comment at: clang/lib/Parse/ParseStmt.cpp:218
  DeclEnd, Attrs);
+  if (SeenGNUAttributes) {
+DeclStart = GNUAttributeLoc;

Elide braces.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1279
 continue;
-  if (S.getLangOpts().CPlusPlus11) {
+  if (S.getLangOpts().CPlusPlus11 || S.getLangOpts().C99) {
 const Stmt *Term = B->getTerminatorStmt();

What is special about C99 here?


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

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Index/blocks.c
  clang/test/Index/load-exprs.c
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp
  clang/test/SemaCXX/warn-unused-label-error.cpp

Index: clang/test/SemaCXX/warn-unused-label-error.cpp
===
--- clang/test/SemaCXX/warn-unused-label-error.cpp
+++ clang/test/SemaCXX/warn-unused-label-error.cpp
@@ -18,9 +18,9 @@
   }
 
   void h() {
-D: // expected-warning {{unused label 'D'}}
+D:
   #pragma weak unused_local_static
-  __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
+  __attribute__((unused))  // expected-error {{'unused' attribute cannot be applied to a statement}}
   ;
   }
 }
Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+
+int foo(int x) {
+  int a = 0;
+
+  switch (x) {
+  case 0:
+a++;
+  case 1:
+// expected-warning@-1{{unannotated fall-through between switch labels}}
+//expected-note@-2{{insert 'break;' to avoid fall-through}}
+a--;
+  case 2:
+// expected-warning@-1{{unannotated fall-through between switch labels}}
+// expected-note@-2{{insert 'break;' to avoid fall-through}}
+break;
+  default:
+a = 1;
+  }
+
+  return 0;
+}
+
+int bar(int x) {
+  int a = 0;
+
+  switch (x) {
+  case 0:
+a++;
+__attribute__((fallthrough));
+  case 1:
+a--;
+__attribute__((fallthrough));
+  case 2:
+break;
+  default:
+a = 1;
+  }
+
+  return 0;
+}
+
+__attribute__((fallthrough)); // expected-warning {{declaration does not declare anything}}
+void baz(int x) {
+  __attribute__((fallthrough)); // expected-error {{fallthrough annotation is outside switch statement}}
+}
Index: clang/test/Index/load-exprs.c
===
--- clang/test/Index/load-exprs.c
+++ clang/test/Index/load-exprs.c
@@ -52,7 +52,7 @@
 // CHECK: load-exprs.c:7:23: DeclRefExpr=x:6:12 Extent=[7:23 - 7:24]
 // CHECK: load-exprs.c:10:5: FunctionDecl=test_blocks:10:5 (Definition) Extent=[10:1 - 21:2]
 // CHECK: load-exprs.c:10:21: ParmDecl=x:10:21 (Definition) Extent=[10:17 - 10:22]
-// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:3 - 11:20]
+// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:11 - 11:20]
 // CHECK: load-exprs.c:11:19: DeclRefExpr=x:10:21 Extent=[11:19 - 11:20]
 // CHECK: load-exprs.c:12:3: CallExpr= Extent=[12:3 - 19:7]
 // CHECK: load-exprs.c:13:17: VarDecl=z:13:17 (Definition) Extent=[13:6 - 13:22]
Index: clang/test/Index/blocks.c
===
--- clang/test/Index/blocks.c
+++ clang/test/Index/blocks.c
@@ -14,7 +14,7 @@
 // CHECK: blocks.c:7:3: DeclStmt= Extent=[7:3 - 7:26]
 // CHECK: blocks.c:7:21: VarDecl=_foo:7:21 (Definition) Extent=[7:3 - 7:25]
 // CHECK: blocks.c:7:17: TypeRef=struct foo:4:8 Extent=[7:17 - 7:20]
-// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:3 - 8:16]
+// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:11 - 8:16]
 // CHECK: blocks.c:8:15: IntegerLiteral= Extent=[8:15 - 8:16]
 // CHECK: blocks.c:9:3: CallExpr= Extent=[9:3 - 9:65]
 // CHECK: blocks.c:9:3: BlockExpr= Extent=[9:3 - 9:58]
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1215,7 +1215,7 @@
 tok::r_square, tok::r_square
   };
 
-  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17;
+  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17 && !PP.getLangOpts().C2x;
 
   StringRef MacroName;
   if (PreferClangAttr)
@@ -1224,24 +1224,19 @@
 MacroName = PP.getLastMacroWithSpelling(Loc, FallthroughTokens);
   if (MacroName.empty() && !PreferClangAttr)
 MacroName = PP.getLastMacroWithSpelling(Loc, 

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Index/blocks.c
  clang/test/Index/load-exprs.c
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp
  clang/test/SemaCXX/warn-unused-label-error.cpp

Index: clang/test/SemaCXX/warn-unused-label-error.cpp
===
--- clang/test/SemaCXX/warn-unused-label-error.cpp
+++ clang/test/SemaCXX/warn-unused-label-error.cpp
@@ -18,9 +18,9 @@
   }
 
   void h() {
-D: // expected-warning {{unused label 'D'}}
-  #pragma weak unused_local_static
-  __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
-  ;
+  D:
+#pragma weak unused_local_static
+__attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
+;
   }
 }
Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+
+int foo(int x) {
+  int a = 0;
+
+  switch (x) {
+  case 0:
+a++;
+  case 1:
+// expected-warning@-1{{unannotated fall-through between switch labels}}
+//expected-note@-2{{insert 'break;' to avoid fall-through}}
+a--;
+  case 2:
+// expected-warning@-1{{unannotated fall-through between switch labels}}
+// expected-note@-2{{insert 'break;' to avoid fall-through}}
+break;
+  default:
+a = 1;
+  }
+
+  return 0;
+}
+
+int bar(int x) {
+  int a = 0;
+
+  switch (x) {
+  case 0:
+a++;
+__attribute__((fallthrough));
+  case 1:
+a--;
+__attribute__((fallthrough));
+  case 2:
+break;
+  default:
+a = 1;
+  }
+
+  return 0;
+}
+
+__attribute__((fallthrough)); // expected-warning {{declaration does not declare anything}}
+void baz(int x) {
+  __attribute__((fallthrough)); // expected-error {{fallthrough annotation is outside switch statement}}
+}
Index: clang/test/Index/load-exprs.c
===
--- clang/test/Index/load-exprs.c
+++ clang/test/Index/load-exprs.c
@@ -52,7 +52,7 @@
 // CHECK: load-exprs.c:7:23: DeclRefExpr=x:6:12 Extent=[7:23 - 7:24]
 // CHECK: load-exprs.c:10:5: FunctionDecl=test_blocks:10:5 (Definition) Extent=[10:1 - 21:2]
 // CHECK: load-exprs.c:10:21: ParmDecl=x:10:21 (Definition) Extent=[10:17 - 10:22]
-// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:3 - 11:20]
+// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:11 - 11:20]
 // CHECK: load-exprs.c:11:19: DeclRefExpr=x:10:21 Extent=[11:19 - 11:20]
 // CHECK: load-exprs.c:12:3: CallExpr= Extent=[12:3 - 19:7]
 // CHECK: load-exprs.c:13:17: VarDecl=z:13:17 (Definition) Extent=[13:6 - 13:22]
@@ -78,4 +78,3 @@
 // CHECK: load-exprs.c:31:32: MemberRef=array:24:12 Extent=[31:32 - 31:37]
 // CHECK: load-exprs.c:31:38: DeclRefExpr=StartIndex:27:8 Extent=[31:38 - 31:48]
 // CHECK: load-exprs.c:31:50: MemberRef=b:2:19 Extent=[31:50 - 31:51]
-
Index: clang/test/Index/blocks.c
===
--- clang/test/Index/blocks.c
+++ clang/test/Index/blocks.c
@@ -14,7 +14,7 @@
 // CHECK: blocks.c:7:3: DeclStmt= Extent=[7:3 - 7:26]
 // CHECK: blocks.c:7:21: VarDecl=_foo:7:21 (Definition) Extent=[7:3 - 7:25]
 // CHECK: blocks.c:7:17: TypeRef=struct foo:4:8 Extent=[7:17 - 7:20]
-// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:3 - 8:16]
+// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:11 - 8:16]
 // CHECK: blocks.c:8:15: IntegerLiteral= Extent=[8:15 - 8:16]
 // CHECK: blocks.c:9:3: CallExpr= Extent=[9:3 - 9:65]
 // CHECK: blocks.c:9:3: BlockExpr= Extent=[9:3 - 9:58]
@@ -31,4 +31,3 @@
 // CHECK: blocks.c:9:54: DeclRefExpr=i:8:11 Extent=[9:54 - 9:55]
 // CHECK: blocks.c:9:59: UnaryOperator= Extent=[9:59 - 9:64]
 // CHECK: blocks.c:9:60: DeclRefExpr=_foo:7:21 Extent=[9:60 - 9:64]
-
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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 parsing once and falling back if 
> parsing fails?
>  Do we want to parse attributes first and then wait until we see a 
> decl-specifier (breaking the implicit int case)?


I don't think so. A GCC attribute is a decl-specifier, so should trigger 
implicit-int in the languages that have it.

Option 1: teach the statement/declaration disambiguation code that an initial 
GNU attribute does not resolve the ambiguity and that it needs to disambiguate 
past one.

Option 2: parse the attributes and then call the disambiguation code and tell 
it that we've already consumed a decl-specifier.


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

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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 and then wait until we see a 
decl-specifier (breaking the implicit int case)?


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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 terms of compile time either.
>
> If we really care about attributes on implicit ints: I don’t think that 
> parsing according to attribute name is so bad solution - if only “possible” 
> issue is same attr. name for stmt and decl for some future attribute - let’s 
> talk with GCC devs and make a deal about it.


Richard and I spoke about that offline last week and both agree that changing 
parsing behavior according to the attribute name is not acceptable behavior for 
the parser.


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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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 think that parsing 
according to attribute name is so bad solution - if only “possible” issue is 
same attr. name for stmt and decl for some future attribute - let’s talk with 
GCC devs and make a deal about it.


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

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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;
> >   }
> >
> >
> > If the attributes are parsed then function body looks like this to the 
> > parser:
> >
> >   {
> > *x; //this one has attributes now
> > *y;
> >   {
> >
> >
> > The first line should be a valid declaration and the second like should be 
> > a dereference of an uninitialized variable. If the attributes token is 
> > discarded before parsing the rest of the line the only way to differentiate 
> > these is by looking at the attributes added to them.
> >
> > An alternative may be parse the attributes list and immediately try to 
> > parse as a declaration then if that parsing fails attempt to parse as 
> > something else. Although this approach also has the scary implication of 
> > things that are supposed to be declarations getting reparsed as something 
> > entirely different.
>
>
> The issue is that a leading GNU-style attribute is not sufficient information 
> to determine whether we're parsing a declaration or a statement; it shouldn't 
> always be treated as a decl-specifier. I spoke with a GCC dev about how they 
> handle this, and effectively, they parse the attributes first then attempt to 
> parse a declaration; if that fails, they fall back to parsing a statement. I 
> think the way forward for us that should be similar is to parse the 
> attributes first and then wait until we see a decl-specifier before 
> determining whether we want to parse a declaration or a statement, and attach 
> the attributes after we've figured out which production we have. @rsmith may 
> have even better approaches in mind, but we're definitely agreed that we 
> should not parse statement/decl based on attribute identity. I would hope we 
> could find a way to avoid lots of re-parsing work if we can (even to the 
> point of perhaps breaking the `address_space` case because implicit int is 
> pretty horrible to rely on in the first place; it depends on whether breaking 
> that will break a lot of code or not).


@xbolva00's patch https://reviews.llvm.org/D63260?id=204583 essentially does 
that already. I'm not sure how to continue on this patch, several solutions 
have been suggested, but I'm not sure which to implement.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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:
>
>   {
> *x; //this one has attributes now
> *y;
>   {
>
>
> The first line should be a valid declaration and the second like should be a 
> dereference of an uninitialized variable. If the attributes token is 
> discarded before parsing the rest of the line the only way to differentiate 
> these is by looking at the attributes added to them.
>
> An alternative may be parse the attributes list and immediately try to parse 
> as a declaration then if that parsing fails attempt to parse as something 
> else. Although this approach also has the scary implication of things that 
> are supposed to be declarations getting reparsed as something entirely 
> different.


The issue is that a leading GNU-style attribute is not sufficient information 
to determine whether we're parsing a declaration or a statement; it shouldn't 
always be treated as a decl-specifier. I spoke with a GCC dev about how they 
handle this, and effectively, they parse the attributes first then attempt to 
parse a declaration; if that fails, they fall back to parsing a statement. I 
think the way forward for us that should be similar is to parse the attributes 
first and then wait until we see a decl-specifier before determining whether we 
want to parse a declaration or a statement, and attach the attributes after 
we've figured out which production we have. @rsmith may have even better 
approaches in mind, but we're definitely agreed that we should not parse 
statement/decl based on attribute identity. I would hope we could find a way to 
avoid lots of re-parsing work if we can (even to the point of perhaps breaking 
the `address_space` case because implicit int is pretty horrible to rely on in 
the first place; it depends on whether breaking that will break a lot of code 
or not).


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

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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 second like should be a 
dereference of an uninitialized variable. If the attributes token is discarded 
before parsing the rest of the line the only way to differentiate these is by 
looking at the attributes added to them.

An alternative may be parse the attributes list and immediately try to parse as 
a declaration then if that parsing fails attempt to parse as something else. 
Although this approach also has the scary implication of things that are 
supposed to be declarations getting reparsed as something entirely different.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D64838#1592118 , 
@Nathan-Huckleberry wrote:

> In D64838#1592111 , @aaron.ballman 
> wrote:
>
> > 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 the attributes list.
> > >
> > > This case demonstrates the problem:
> > >
> > >   void foo() {
> > > __attribute__((address_space(0))) *x;
> > >   }
> > >
> > >
> > > If the attribute token is read beforehand this code just becomes `*x` and 
> > > is parsed as a dereference of an undefined variable when it should 
> > > actually be a declaration.
> > >
> > > Maybe the best solution is to pull the attributes parsing out to 
> > > `ParseStatementOrDeclaration` then pass those attributes through to 
> > > following functions. 
> > >  When the determination is made whether a line is a declaration or a 
> > > statement the attributes list can be looked at to determine if the 
> > > attributes are statement or declaration attributes and continue 
> > > accordingly. Maybe throwing a warning if mixing of declaration and 
> > > statement attributes occur.
> > >
> > > Does that sound like a good solution?
> >
> >
> > Please see the discussion in https://reviews.llvm.org/D63299#inline-564887 
> > -- if I understand your approach properly, this approach leads to spooky 
> > action at a distance, where the name of the attribute dictates whether 
> > something is a statement or a declaration. I'm not comfortable with that. 
> > There's no reason we could not have an attribute that is both a statement 
> > attribute and a declaration attribute, and how would *that* parse?
> >
> > I think this requires some deeper surgery in the parsing logic so that 
> > attributes do not impact whether something is treated as a declaration or a 
> > statement. The parser should parse attributes first, keep them around for a 
> > while, and attach them to either the decl or the statement at a later point.
>
>
> Any idea how to fix the problem in the code sample I gave? The addition of 
> the attributes token causes code to be read as a declaration and without the 
> token it is read as a unary operator.


I would start by trying to have 
`Parser::ParseStatementOrDeclarationAfterAttributes()` parse the GNU-style 
attributes if the `__attribute__` token is encountered, adding to the `Attrs`, 
and then retrying the loop.


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

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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 
> > 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() {
> > __attribute__((address_space(0))) *x;
> >   }
> >
> >
> > If the attribute token is read beforehand this code just becomes `*x` and 
> > is parsed as a dereference of an undefined variable when it should actually 
> > be a declaration.
> >
> > Maybe the best solution is to pull the attributes parsing out to 
> > `ParseStatementOrDeclaration` then pass those attributes through to 
> > following functions. 
> >  When the determination is made whether a line is a declaration or a 
> > statement the attributes list can be looked at to determine if the 
> > attributes are statement or declaration attributes and continue 
> > accordingly. Maybe throwing a warning if mixing of declaration and 
> > statement attributes occur.
> >
> > Does that sound like a good solution?
>
>
> Please see the discussion in https://reviews.llvm.org/D63299#inline-564887 -- 
> if I understand your approach properly, this approach leads to spooky action 
> at a distance, where the name of the attribute dictates whether something is 
> a statement or a declaration. I'm not comfortable with that. There's no 
> reason we could not have an attribute that is both a statement attribute and 
> a declaration attribute, and how would *that* parse?
>
> I think this requires some deeper surgery in the parsing logic so that 
> attributes do not impact whether something is treated as a declaration or a 
> statement. The parser should parse attributes first, keep them around for a 
> while, and attach them to either the decl or the statement at a later point.


Any idea how to fix the problem in the code sample I gave? The addition of the 
attributes token causes code to be read as a declaration and without the token 
it is read as a unary operator.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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 the attributes list.
>
> This case demonstrates the problem:
>
>   void foo() {
> __attribute__((address_space(0))) *x;
>   }
>
>
> If the attribute token is read beforehand this code just becomes `*x` and is 
> parsed as a dereference of an undefined variable when it should actually be a 
> declaration.
>
> Maybe the best solution is to pull the attributes parsing out to 
> `ParseStatementOrDeclaration` then pass those attributes through to following 
> functions. 
>  When the determination is made whether a line is a declaration or a 
> statement the attributes list can be looked at to determine if the attributes 
> are statement or declaration attributes and continue accordingly. Maybe 
> throwing a warning if mixing of declaration and statement attributes occur.
>
> Does that sound like a good solution?


Please see the discussion in https://reviews.llvm.org/D63299#inline-564887 -- 
if I understand your approach properly, this approach leads to spooky action at 
a distance, where the name of the attribute dictates whether something is a 
statement or a declaration. I'm not comfortable with that. There's no reason we 
could not have an attribute that is both a statement attribute and a 
declaration attribute, and how would *that* parse?

I think this requires some deeper surgery in the parsing logic so that 
attributes do not impact whether something is treated as a declaration or a 
statement. The parser should parse attributes first, keep them around for a 
while, and attach them to either the decl or the statement at a later point.




Comment at: clang/lib/Parse/ParseStmt.cpp:104
+
+  if (isNullStmtWithAttributes()) {
+MaybeParseGNUAttributes(Attrs);

efriedma wrote:
> If we're going to generally support statement attributes, it should be 
> possible to apply them to non-null statements.  (Even if there aren't any 
> valid attributes right now, that's likely to change in the future.)  Or is 
> that not possible to implement for some reason?
We already generally support statement attributes. See `ProcessStmtAttribute()` 
in SemaStmtAttr.cpp. They definitely need to apply to non-null statements.


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

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

[Eli Friedman via Phabricator via cfe-commits]

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

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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() {
__attribute__((address_space(0))) *x;
  }

If the attribute token is read beforehand this code just becomes `*x` and is 
parsed as a dereference of an undefined variable when it should actually be a 
declaration.

Maybe the best solution is to pull the attributes parsing out to 
`ParseStatementOrDeclaration` then pass those attributes through to following 
functions. 
When the determination is made whether a line is a declaration or a statement 
the attributes list can be looked at to determine if the attributes are 
statement or declaration attributes and continue accordingly. Maybe throwing a 
warning if mixing of declaration and statement attributes occur.

Does that sound like a good solution?


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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 actions in a 
tentatively-parsed region; they might annotate or otherwise mess with the token 
stream in undesirable ways, or produce diagnostics, etc. If we keep this 
tentative parsing formulation, you'll need to instead recognize the 
`__attribute__` token then manually skip its attribute list.



Comment at: clang/lib/Parse/ParseTentative.cpp:2146
+bool Parser::isNullStmtWithAttributes() {
+  RevertingTentativeParsingAction PA(*this);
+  return TryParseNullStmtWithAttributes() == TPResult::True;

xbolva00 wrote:
> Is this “cheap” in terms of compile time?
No; this is not a reasonable thing to do for every block-scope statement or 
declaration that we parse.

We should do the same thing that we do for all other kinds of attribute: parse 
them unconditionally then reject them in the contexts where they should not be 
permitted.


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

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

[Eli Friedman via Phabricator via cfe-commits]

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 statements.  (Even if there aren't any valid 
attributes right now, that's likely to change in the future.)  Or is that not 
possible to implement for some reason?


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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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 compile time?


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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D64838/new/

https://reviews.llvm.org/D64838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1280
+  if (S.getLangOpts().CPlusPlus11 || S.getLangOpts().C99) {
 const Stmt *Term = B->getTerminatorStmt();
 // Skip empty cases.

Make it unconditional (see my patch)


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

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/ParseTentative.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp

Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+
+int foo(int x) {
+  int a = 0;
+
+  switch (x) {
+  case 0:
+a++;
+  case 1:
+// expected-warning@-1{{unannotated fall-through between switch labels}}
+//expected-note@-2{{insert 'break;' to avoid fall-through}}
+a--;
+  case 2:
+// expected-warning@-1{{unannotated fall-through between switch labels}}
+// expected-note@-2{{insert 'break;' to avoid fall-through}}
+break;
+  default:
+a = 1;
+  }
+
+  return 0;
+}
+
+int bar(int x) {
+  int a = 0;
+
+  switch (x) {
+  case 0:
+a++;
+__attribute__((fallthrough));
+  case 1:
+a--;
+__attribute__((fallthrough));
+  case 2:
+break;
+  default:
+a = 1;
+  }
+
+  return 0;
+}
+
+__attribute__((fallthrough)); // expected-warning {{declaration does not declare anything}}
+void baz(int x) {
+  __attribute__((fallthrough)); // expected-error {{fallthrough annotation is outside switch statement}}
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1215,7 +1215,7 @@
 tok::r_square, tok::r_square
   };
 
-  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17;
+  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17 && !PP.getLangOpts().C2x;
 
   StringRef MacroName;
   if (PreferClangAttr)
@@ -1224,24 +1224,19 @@
 MacroName = PP.getLastMacroWithSpelling(Loc, FallthroughTokens);
   if (MacroName.empty() && !PreferClangAttr)
 MacroName = PP.getLastMacroWithSpelling(Loc, ClangFallthroughTokens);
-  if (MacroName.empty())
-MacroName = PreferClangAttr ? "[[clang::fallthrough]]" : "[[fallthrough]]";
+  if (MacroName.empty()) {
+if (!PreferClangAttr)
+  MacroName = "[[fallthrough]]";
+else if (PP.getLangOpts().CPlusPlus)
+  MacroName = "[[clang::fallthrough]]";
+else
+  MacroName = "__attribute__((fallthrough))";
+  }
   return MacroName;
 }
 
 static void DiagnoseSwitchLabelsFallthrough(Sema , AnalysisDeclContext ,
 bool PerFunction) {
-  // Only perform this analysis when using [[]] attributes. There is no good
-  // workflow for this warning when not using C++11. There is no good way to
-  // silence the warning (no attribute is available) unless we are using
-  // [[]] attributes. One could use pragmas to silence the warning, but as a
-  // general solution that is gross and not in the spirit of this warning.
-  //
-  // NOTE: This an intermediate solution. There are on-going discussions on
-  // how to properly support this warning outside of C++11 with an annotation.
-  if (!AC.getASTContext().getLangOpts().DoubleSquareBracketAttributes)
-return;
-
   FallthroughMapper FM(S);
   FM.TraverseStmt(AC.getBody());
 
@@ -1281,7 +1276,7 @@
   SourceLocation L = Label->getBeginLoc();
   if (L.isMacroID())
 continue;
-  if (S.getLangOpts().CPlusPlus11) {
+  if (S.getLangOpts().CPlusPlus11 || S.getLangOpts().C99) {
 const Stmt *Term = B->getTerminatorStmt();
 // Skip empty cases.
 while (B->empty() && !Term && B->succ_size() == 1) {
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -2121,3 +2121,28 @@
 return TPResult::Ambiguous;
   return TPResult::False;
 }
+
+Parser::TPResult Parser::TryParseNullStmtWithAttributes() {
+  if (Tok.isNot(tok::kw___attribute)) {
+return TPResult::False;
+  }
+  ParsedAttributesWithRange attrs(AttrFactory);
+  ParseGNUAttributes(attrs, nullptr, nullptr);
+  if (attrs.size() <= 0) {
+return TPResult::False;
+  }
+  for (auto  : 

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

[Nathan Huckleberry via Phabricator via cfe-commits]

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
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/ParseTentative.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/Sema/fallthrough-attr.c
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp

Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -329,3 +329,15 @@
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+n++;
+__attribute__ ((fallthrough));
+  case 1:
+n++;
+break;
+  }
+  return n;
+}
Index: clang/test/Sema/fallthrough-attr.c
===
--- /dev/null
+++ clang/test/Sema/fallthrough-attr.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+
+int foo(int x)
+{
+int a = 0;
+
+switch (x) {
+case 0:
+  a++;
+case 1:
+  // expected-warning@-1{{unannotated fall-through between switch labels}}
+  //expected-note@-2{{insert 'break;' to avoid fall-through}}
+  a--;
+case 2:
+  // expected-warning@-1{{unannotated fall-through between switch labels}}
+  // expected-note@-2{{insert 'break;' to avoid fall-through}}
+break;
+default:
+a = 1;
+}
+
+return 0;
+}
+
+int bar(int x)
+{
+int a = 0;
+
+switch (x) {
+case 0:
+  a++;
+  __attribute__ ((fallthrough));
+case 1:
+  a--;
+  __attribute__ ((fallthrough));
+case 2:
+break;
+default:
+a = 1;
+}
+
+return 0;
+}
+
+__attribute__ ((fallthrough)); // expected-warning {{declaration does not declare anything}}
+void baz(int x) {
+  __attribute__ ((fallthrough)); // expected-error {{fallthrough annotation is outside switch statement}}
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1215,7 +1215,7 @@
 tok::r_square, tok::r_square
   };
 
-  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17;
+  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17 && !PP.getLangOpts().C2x;
 
   StringRef MacroName;
   if (PreferClangAttr)
@@ -1224,24 +1224,19 @@
 MacroName = PP.getLastMacroWithSpelling(Loc, FallthroughTokens);
   if (MacroName.empty() && !PreferClangAttr)
 MacroName = PP.getLastMacroWithSpelling(Loc, ClangFallthroughTokens);
-  if (MacroName.empty())
-MacroName = PreferClangAttr ? "[[clang::fallthrough]]" : "[[fallthrough]]";
+  if (MacroName.empty()) {
+if (!PreferClangAttr)
+  MacroName = "[[fallthrough]]";
+else if (PP.getLangOpts().CPlusPlus)
+  MacroName = "[[clang::fallthrough]]";
+else
+  MacroName = "__attribute__((fallthrough))";
+  }
   return MacroName;
 }
 
 static void DiagnoseSwitchLabelsFallthrough(Sema , AnalysisDeclContext ,
 bool PerFunction) {
-  // Only perform this analysis when using [[]] attributes. There is no good
-  // workflow for this warning when not using C++11. There is no good way to
-  // silence the warning (no attribute is available) unless we are using
-  // [[]] attributes. One could use pragmas to silence the warning, but as a
-  // general solution that is gross and not in the spirit of this warning.
-  //
-  // NOTE: This an intermediate solution. There are on-going discussions on
-  // how to properly support this warning outside of C++11 with an annotation.
-  if (!AC.getASTContext().getLangOpts().DoubleSquareBracketAttributes)
-return;
-
   FallthroughMapper FM(S);
   FM.TraverseStmt(AC.getBody());
 
@@ -1281,7 +1276,7 @@
   SourceLocation L = Label->getBeginLoc();
   if (L.isMacroID())
 continue;
-  if (S.getLangOpts().CPlusPlus11) {
+  if (S.getLangOpts().CPlusPlus11 || S.getLangOpts().C99) {
 const Stmt *Term = B->getTerminatorStmt();
 // Skip empty cases.
 while (B->empty() && !Term && B->succ_size() == 1) {
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -2121,3 +2121,28 @@
 return TPResult::Ambiguous;
   return TPResult::False;
 }
+
+Parser::TPResult Parser::TryParseNullStmtWithAttributes() {
+  if(Tok.isNot(tok::kw___attribute)) {
+return TPResult::False;
+