aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM! Do you need me to commit on your behalf? If so, what name and email
address would you like used for patch attribution?
================
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();
}
}
----------------
alejandro-alvarez-sonarsource wrote:
> 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?
I also lean towards not modifying `SkipMalformedDecl()`; I think it's recovery
strategy is sensible.
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
