> On Oct 5, 2011, at 5:25 PM, Richard Smith wrote:
>> On Thu, October 6, 2011 00:58, Eli Friedman wrote:
>>> On Wed, Oct 5, 2011 at 4:26 PM, Richard Smith <[email protected]>
>>> wrote:
>>>> Clang currently gives unhelpful diagnostics for cases such as this:
>>>>
>>>> static const char *const Triples[] = { "powerpc-linux-gnu",
>>>> "powerpc-unknown-linux-gnu"
>>>> },
>>>> CandidateLibDirs.append(LibDirs, LibDirs +
>>>> llvm::array_lengthof(LibDirs));
>>>>
>>>> Viz:
>>>>
>>>> lib/Driver/ToolChains.cpp:1582:7: error: default initialization of an
>>>> object of const type 'const char' CandidateLibDirs.append(LibDirs,
>>>> LibDirs + llvm::array_lengthof(LibDirs));
>>>> ^
>>>> lib/Driver/ToolChains.cpp:1582:23: error: expected ';' at end of
>>>> declaration CandidateLibDirs.append(LibDirs, LibDirs +
>>>> llvm::array_lengthof(LibDirs));
>>>> ^
>>>> ;
>>>>
>>>>
>>>> The attached patch is a conservative fix for this issue. In cases where
>>>> a declarator group contains a comma followed by a newline followed by
>>>> something which obviously is neither a declarator nor a typo for a
>>>> declarator, we give a fixit suggesting that a ; was intended:
>>>>
>>>> lib/Driver/ToolChains.cpp:1581:8: error: expected ';' at end of
>>>> declaration },
>>>> ^ ;
>>>>
>>>> OK to commit?
>>>
>>> I don't really like Parser::MightBeDeclarator... I can see at least
>>> two cases where it rejects valid code. Can you use
>>> Parser::TryParseDeclarator
>>> or something?
>>
>> I'd be reluctant to add tentative parsing for all declarator groups with
>> more than one declarator.
On Thu, October 6, 2011 01:44, John McCall wrote:
> Yes, please don't use tentative parsing here. :) In fact, if you could use
> at most one token of lookahead, that would be ideal.
Indeed, that is how the patch operates.
> Abstractly, I don't really like the approach of opting-out possible
> declarators, but the alternative would make the diagnostic *very*
> special-case. Please at least leave a comment in the declarator-parsing code
> saying to update your function if a new token is added there, though.
I'm not hugely enamored with the approach either, but I think a black-list of
"obvious" non-declarators would be worse. I've added a comment as you
requested.
On Thu, October 6, 2011 01:59, Eli Friedman wrote:
> On Wed, Oct 5, 2011 at 5:25 PM, Richard Smith <[email protected]> wrote:
>> What are your cases? Even if we change approach, I'd like to add tests for
>> them.
>
> Err, hmm, one of them wasn't actually right. I'm pretty sure the
> following is valid, though:
>
> int x, y alignas(float);
Thanks! I'd accidentally added kw_alignas to the 'before identifier' list
rather than the 'after identifier' list where it belongs.
The attached patch fixes that, adds support for code_completion tokens, and
adds a few more cases to the whitelist where the existing error recovery was
better than the new error recovery (for instance, in "{ int a, \n b }"). I've
gone over the declaration parsing code again to check I didn't miss any
tokens, and tested this against a very large quantity of C++ code with no
rejects-valids. Further comments welcome!
Thanks,
RichardIndex: test/Parser/cxx0x-decl.cpp
===================================================================
--- test/Parser/cxx0x-decl.cpp (revision 0)
+++ test/Parser/cxx0x-decl.cpp (revision 0)
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++0x %s
+
+// Make sure we know these are legitimate commas and not typos for ';'.
+namespace Commas {
+ int a,
+ b [[ ]],
+ c alignas(double);
+}
Index: test/Parser/cxx-decl.cpp
===================================================================
--- test/Parser/cxx-decl.cpp (revision 141241)
+++ test/Parser/cxx-decl.cpp (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fsyntax-only %s
+// RUN: %clang_cc1 -verify -fsyntax-only -triple i386-linux %s
int x(*g); // expected-error {{use of undeclared identifier 'g'}}
@@ -66,6 +66,36 @@
int z // expected-error {{expected ';' at end of declaration list}}
};
+// Make sure we know these are legitimate commas and not typos for ';'.
+namespace Commas {
+ struct S {
+ static int a;
+ int c,
+ operator()();
+ };
+
+ int global1,
+ __attribute__(()) global2,
+ (global5),
+ *global6,
+ &global7 = global1,
+ &&global8 = static_cast<int&&>(global1), // expected-warning 2{{rvalue reference}}
+ S::a,
+ global9,
+ global10 = 0,
+ global11 == 0, // expected-error {{did you mean '='}}
+ global12 __attribute__(()),
+ global13(0),
+ global14[2],
+ global15;
+
+ void g() {
+ static int a,
+ b __asm__("ebx"), // expected-error {{expected ';' at end of declaration}}
+ Statics:return;
+ }
+}
+
// PR5825
struct test5 {};
::new(static_cast<void*>(0)) test5; // expected-error {{expected unqualified-id}}
Index: test/FixIt/fixit.cpp
===================================================================
--- test/FixIt/fixit.cpp (revision 141241)
+++ test/FixIt/fixit.cpp (working copy)
@@ -107,4 +107,16 @@
aPtr = br; // expected-error {{assigning to 'A *' from incompatible type 'B'; take the address with &}}
}
-
+struct S { void f(int, char); };
+int itsAComma,
+itsAComma2 = 0,
+oopsAComma(42), // expected-error {{expected ';' after declaration}}
+AD oopsMoreCommas() {
+ static int n = 0,
+ static char c,
+ &d = c, // expected-error {{expected ';' after declaration}}
+ S s, // expected-error {{expected ';' after declaration}}
+ s.f(n, d);
+ AD ad, // expected-error {{expected ';' after declaration}}
+ return ad;
+}
Index: test/FixIt/fixit.c
===================================================================
--- test/FixIt/fixit.c (revision 141241)
+++ test/FixIt/fixit.c (working copy)
@@ -70,3 +70,10 @@
: c++;
c = c + 3; L4: return;
}
+
+int oopsAComma = 0,
+void oopsMoreCommas() {
+ static int a[] = { 0, 1, 2 },
+ static int b[] = { 3, 4, 5 },
+ &a == &b ? oopsMoreCommas() : removeUnusedLabels(a[0]);
+}
Index: test/FixIt/fixit-cxx0x.cpp
===================================================================
--- test/FixIt/fixit-cxx0x.cpp (revision 141241)
+++ test/FixIt/fixit-cxx0x.cpp (working copy)
@@ -57,3 +57,9 @@
// -> constexpr static char *const p = 0;
};
}
+
+namespace SemiCommaTypo {
+ int m {},
+ n [[]], // expected-error {{expected ';' at end of declaration}}
+ int o;
+}
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h (revision 141241)
+++ include/clang/Parse/Parser.h (working copy)
@@ -1469,6 +1469,7 @@
ParsedAttributes &attrs,
bool RequireSemi,
ForRangeInit *FRI = 0);
+ bool MightBeDeclarator(unsigned Context);
DeclGroupPtrTy ParseDeclGroup(ParsingDeclSpec &DS, unsigned Context,
bool AllowFunctionDefinitions,
SourceLocation *DeclEnd = 0,
Index: lib/Parse/ParseDecl.cpp
===================================================================
--- lib/Parse/ParseDecl.cpp (revision 141241)
+++ lib/Parse/ParseDecl.cpp (working copy)
@@ -984,6 +984,61 @@
return ParseDeclGroup(DS, Context, /*FunctionDefs=*/ false, &DeclEnd, FRI);
}
+/// Returns true if this might be the start of a declarator, or a common typo
+/// for a declarator.
+bool Parser::MightBeDeclarator(unsigned Context) {
+ switch (Tok.getKind()) {
+ case tok::annot_cxxscope:
+ case tok::annot_template_id:
+ case tok::caret:
+ case tok::code_completion:
+ case tok::coloncolon:
+ case tok::ellipsis:
+ case tok::kw___attribute:
+ case tok::kw_operator:
+ case tok::l_paren:
+ case tok::star:
+ return true;
+
+ case tok::amp:
+ case tok::ampamp:
+ case tok::colon: // Might be a typo for '::'.
+ return getLang().CPlusPlus;
+
+ case tok::identifier:
+ switch (NextToken().getKind()) {
+ case tok::code_completion:
+ case tok::coloncolon:
+ case tok::comma:
+ case tok::equal:
+ case tok::equalequal: // Might be a typo for '='.
+ case tok::kw_alignas:
+ case tok::kw_asm:
+ case tok::kw___attribute:
+ case tok::l_brace:
+ case tok::l_paren:
+ case tok::l_square:
+ case tok::less:
+ case tok::r_brace:
+ case tok::r_paren:
+ case tok::r_square:
+ case tok::semi:
+ return true;
+
+ case tok::colon:
+ // At namespace scope, 'identifier:' is probably a typo for 'identifier::'
+ // and in block scope it's probably a label.
+ return getLang().CPlusPlus && Context == Declarator::FileContext;
+
+ default:
+ return false;
+ }
+
+ default:
+ return false;
+ }
+}
+
/// ParseDeclGroup - Having concluded that this is either a function
/// definition or a group of object declarations, actually parse the
/// result.
@@ -1061,12 +1116,23 @@
if (FirstDecl)
DeclsInGroup.push_back(FirstDecl);
+ bool ExpectSemi = Context != Declarator::ForContext;
+
// If we don't have a comma, it is either the end of the list (a ';') or an
// error, bail out.
while (Tok.is(tok::comma)) {
- // Consume the comma.
- ConsumeToken();
+ SourceLocation CommaLoc = ConsumeToken();
+ if (Tok.isAtStartOfLine() && ExpectSemi && !MightBeDeclarator(Context)) {
+ // This comma was followed by a line-break and something which can't be
+ // the start of a declarator. The comma was probably a typo for a
+ // semicolon.
+ Diag(CommaLoc, diag::err_expected_semi_declaration)
+ << FixItHint::CreateReplacement(CommaLoc, ";");
+ ExpectSemi = false;
+ break;
+ }
+
// Parse the next declarator.
D.clear();
@@ -1090,7 +1156,7 @@
if (DeclEnd)
*DeclEnd = Tok.getLocation();
- if (Context != Declarator::ForContext &&
+ if (ExpectSemi &&
ExpectAndConsume(tok::semi,
Context == Declarator::FileContext
? diag::err_invalid_token_after_toplevel_declarator
@@ -3544,6 +3610,9 @@
/// isn't parsed at all, making this function effectively parse the C++
/// ptr-operator production.
///
+/// If the grammar of this construct is extended, matching changes must also be
+/// made to TryParseDeclarator and MightBeDeclarator.
+///
/// declarator: [C99 6.7.5] [C++ 8p4, dcl.decl]
/// [C] pointer[opt] direct-declarator
/// [C++] direct-declarator_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
