alejandro-alvarez-sonarsource added inline comments.

================
Comment at: clang/lib/Parse/ParseDecl.cpp:2306-2310
     // Otherwise things are very confused and we skip to recover.
     if (!isDeclarationSpecifier(ImplicitTypenameContext::No)) {
-      SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
-      TryConsumeToken(tok::semi);
+      SkipMalformedDecl();
     }
   }
----------------
aaron.ballman wrote:
> Changes for our coding style.
> 
> There seems to be some unfortunate interplay here though. Consider:
> ```
> void bar() namespace foo { int i; }
> 
> int main() {
>   foo::i = 12;
> }
> ```
> Calling `SkipMalformedDecl()` changes the behavior for this test case because 
> we don't recover as well. With your patch applied, this gives us two 
> diagnostics:
> ```
> C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:11: error: 
> expected ';' after top level declarator
> void bar() namespace foo { int i; }
>           ^
>           ;
> C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:3: error: 
> use of undeclared identifier 'foo'
>   foo::i = 12;
>   ^
> 2 errors generated.
> ```
> If the `namespace` keyword is on its own line, then we recover gracefully and 
> don't emit the "use of undeclared identifier" warning.
> 
> While this is technically a regression in behavior for this test case, I 
> think the overall changes are still an improvement. I suspect not a whole lot 
> of code puts `namespace` somewhere other than the start of a line (same for 
> `inline namespace` which has the same behavior with your patch).
> Calling SkipMalformedDecl() changes the behavior for this test case because 
> we don't recover as well. With your patch applied, this gives us two 
> diagnostics:

True. This can also be recovered by removing `Tok.isAtStartOfLine()` in line 
2050. However, this has been around for a long time and would change the 
behavior of 
two tests inside `test/Parser/recovery.cpp`, although only because the broken 
comments contain the namespace keyword.

Either case seems unlikely to me, so I think I'd lean toward not modifying 
`SkipMalformedDecl`. What do you think?


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

https://reviews.llvm.org/D150258

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D150258... Alejandro Álvarez Ayllón via Phabricator via cfe-commits
    • [PATCH] D1... Aaron Ballman via Phabricator via cfe-commits
    • [PATCH] D1... Alejandro Álvarez Ayllón via Phabricator via cfe-commits
    • [PATCH] D1... Alejandro Álvarez Ayllón via Phabricator via cfe-commits
    • [PATCH] D1... Aaron Ballman via Phabricator via cfe-commits
    • [PATCH] D1... Alejandro Álvarez Ayllón via Phabricator via cfe-commits
    • [PATCH] D1... Aaron Ballman via Phabricator via cfe-commits

Reply via email to