On Mon, Mar 26, 2012 at 2:43 PM, Richard Trieu <[email protected]> wrote:
> On Tue, Feb 14, 2012 at 11:10 AM, Richard Trieu <[email protected]> wrote:
>>
>> On Mon, Feb 13, 2012 at 4:05 PM, David Blaikie <[email protected]> wrote:
>>>
>>> On Mon, Feb 13, 2012 at 3:58 PM, Richard Trieu <[email protected]> wrote:
>>> > On Mon, Feb 13, 2012 at 3:13 PM, David Blaikie <[email protected]>
>>> > wrote:
>>> >>
>>> >> On Mon, Feb 13, 2012 at 2:37 PM, Richard Trieu <[email protected]>
>>> >> wrote:
>>> >> > On Mon, Feb 13, 2012 at 1:59 PM, David Blaikie <[email protected]>
>>> >> > wrote:
>>> >> >>
>>> >> >> On Mon, Feb 13, 2012 at 1:52 PM, Richard Trieu <[email protected]>
>>> >> >> wrote:
>>> >> >> > On Mon, Jan 30, 2012 at 10:40 AM, Richard Trieu
>>> >> >> > <[email protected]>
>>> >> >> > wrote:
>>> >> >> >>
>>> >> >> >> On Wed, Jan 25, 2012 at 5:24 PM, Richard Trieu
>>> >> >> >> <[email protected]>
>>> >> >> >> wrote:
>>> >> >> >>>
>>> >> >> >>> This patch adds a warning to catch semi-colons after function
>>> >> >> >>> definitions. The motivation of removing extraneous semi-colons
>>> >> >> >>> is
>>> >> >> >>> to
>>> >> >> >>> make
>>> >> >> >>> it easier to see which scopes are being ended by a brace.
>>> >> >> >>> Function
>>> >> >> >>> scopes
>>> >> >> >>> will end with a plain brace. Class scopes will end with brace
>>> >> >> >>> and
>>> >> >> >>> semi-colon.
>>> >> >> >>>
>>> >> >> >>> class C {
>>> >> >> >>> void F() {};
>>> >> >> >>> };
>>> >> >> >>>
>>> >> >> >>> extra-semi.cc:2:14: warning: extra ';' after function
>>> >> >> >>> definition
>>> >> >> >>> [-Wextra-semi]
>>> >> >> >>> void F() {};
>>> >> >> >>> ^
>>> >> >> >>> 1 warning generated.
>>> >> >> >>
>>> >> >> >>
>>> >> >> >> Ping.
>>> >> >> >
>>> >> >> > Ping.
>>> >> >>
>>> >> >> Not to derail this review - but just out of curiosity: do we have a
>>> >> >> more general 'unnecessary ;' warning? or could we make a general
>>> >> >> one
>>> >> >> that covers all cases where ';' is just redudndant & suggests a
>>> >> >> removal?
>>> >> >
>>> >> > There's a few in the parse diagnostics. They are:
>>> >> >
>>> >> > ext_top_level_semi
>>> >> > warn_cxx98_compat_top_level_semi
>>> >> > ext_extra_struct_semi
>>> >> > ext_extra_ivar_semi
>>> >> >
>>> >> > warn_cxx98_compat_top_level_semi is a warning that is in a
>>> >> > diagnostic
>>> >> > group
>>> >> > so it must be left separate. The other 3 are Extensions and could
>>> >> > probably
>>> >> > be folded in with my diagnostic if I switched mine to Extension as
>>> >> > well
>>> >> > and
>>> >> > added a select into the message.
>>> >>
>>> >>
>>> >> Oh, sorry - I wasn't so much concerned with the diagnostic text, as
>>> >> the logic - is there some way we could, in a relatively
>>> >> universal/general way, catch all/most unnecessary semicolons rather
>>> >> than adding it on a case-by-case basis like this?
>>> >
>>> >
>>> > Yup, that's also possible. It's usually a simple check like:
>>> >
>>> > if (Tok.is(tok::semi)) {
>>> > // Emit Diagnostic
>>> > ConsumeToken();
>>> > }
>>> >
>>> > And replace it with a ConsumeExtraSemi function call with a partial
>>> > diagnostic argument. Then we can add some more robust logic into semi
>>> > checking. But we'll still need to find every check for a semi-colon
>>> > token
>>> > and add this check there, mainly so the diagnostic has enough context
>>> > to
>>> > emit the right error message.
>>>
>>> Fair enough - though I'm not sure if a generic error message would be
>>> terribly problematic (do you really need to know that this particular
>>> spurious semi is after a member function definition rather than at the
>>> global scope?)
>>>
>>> If the checks are that regular it'd probably make sense to add a
>>> utility function to sema (similar to the brace handling, etc) to
>>> conusme - with an optional diagnostic to emit & a default to use
>>> otherwise. But yeah, I suppose the essential answer to my question, as
>>> it pertains to this CR is "no, there's not one place we can fix this
>>> at the moment - but a pretty common cliche we can sweep to cleanup if
>>> we want to spend the tiime at some ponit"
>>>
>>> Thanks,
>>> - David
>>
>>
>> Took some of David's suggestions and modified the patch. Condensed all
>> the extra semi warnings into a single diagnostic message with a %select and
>> with the flag -Wextra-semi. C++98 compat warning was kept separate. Moved
>> the emitting of logic and semi consumption to a shared function. I also
>> added logic so that lines of semi-colons now generate a single warning
>> instead of a warning per semi-colon.
>
>
> Ping.
Attached an updated patch for ToT.
This mostly looks fine to me. Though I don't quite understand the
"CanStartLine" parameter - is this just to differentiate the
diagnostic for:
void func() {
};
and:
void func() {
}
;
? or is there some other reason for that parameter? When I removed the
check in ConsumeExtraSemi it only caused the Parser/cxx-class.cpp to
fail. (I'm not sure what cxx-class.cpp is meant to test - it seems
like a sort of random grab-bag but perhaps so long as it exists it's
an OK place for you to test those two cases)
[Personally I wouldn't bother differentiating these diagnostics at all
(drop the select, drop the extra contextual parameters to
ConsumeExtraSemi (well I suppose you need the OutsideFunction state
specifically to catch the compat warning)) but if you/others think
they're worthwhile I'm OK with that too. Just my 2c there.]
diff --git include/clang/Basic/DiagnosticParseKinds.td include/clang/Basic/DiagnosticParseKinds.td
index c585f87..83568c4 100644
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -21,15 +21,16 @@ def warn_file_asm_volatile : Warning<
let CategoryName = "Parse Issue" in {
def ext_empty_source_file : Extension<"ISO C forbids an empty source file">;
-def ext_top_level_semi : Extension<
- "extra ';' outside of a function">;
def warn_cxx98_compat_top_level_semi : Warning<
"extra ';' outside of a function is incompatible with C++98">,
InGroup<CXX98CompatPedantic>, DefaultIgnore;
-def ext_extra_struct_semi : Extension<
- "extra ';' inside a %0">;
-def ext_extra_ivar_semi : Extension<
- "extra ';' inside instance variable list">;
+def ext_extra_semi : Extension<
+ "extra ';' %select{"
+ "outside of a function|"
+ "inside a %1|"
+ "inside instance variable list|"
+ "after function definition}0">,
+ InGroup<DiagGroup<"extra-semi">>;
def ext_duplicate_declspec : Extension<"duplicate '%0' declaration specifier">;
def ext_plain_complex : ExtWarn<
diff --git include/clang/Parse/Parser.h include/clang/Parse/Parser.h
index 892d5fa..291ec53 100644
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -644,6 +644,21 @@ private:
/// to the semicolon, consumes that extra token.
bool ExpectAndConsumeSemi(unsigned DiagID);
+ /// \brief The kind of extra semi diagnostic to emit.
+ enum ExtraSemiKind {
+ OutsideFunction = 0,
+ InsideStruct = 1,
+ InstanceVariableList = 2,
+ AfterDefinition = 3
+ };
+
+ /// \brief Consume any extra semi-colons until the end of the line.
+ ///
+ /// If CanStartLine is true, then the first semi-colon can begin a line.
+ /// Otherwise, defer consuming semi-colon until a later time.
+ void ConsumeExtraSemi(ExtraSemiKind Kind, bool CanStartLine,
+ const char* DiagMsg = "");
+
//===--------------------------------------------------------------------===//
// Scope manipulation
diff --git lib/Parse/ParseDecl.cpp lib/Parse/ParseDecl.cpp
index 24386d0..dc20ac9 100644
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -2518,10 +2518,8 @@ void Parser::ParseStructUnionBody(SourceLocation RecordLoc,
// Check for extraneous top-level semicolon.
if (Tok.is(tok::semi)) {
- Diag(Tok, diag::ext_extra_struct_semi)
- << DeclSpec::getSpecifierName((DeclSpec::TST)TagType)
- << FixItHint::CreateRemoval(Tok.getLocation());
- ConsumeToken();
+ ConsumeExtraSemi(InsideStruct, /*CanStartLine*/true,
+ DeclSpec::getSpecifierName((DeclSpec::TST)TagType));
continue;
}
diff --git lib/Parse/ParseDeclCXX.cpp lib/Parse/ParseDeclCXX.cpp
index 9321c13..1bb9925 100644
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -1911,9 +1911,8 @@ void Parser::ParseCXXClassMemberDeclaration(AccessSpecifier AS,
LateParsedAttrs.clear();
// Consume the ';' - it's optional unless we have a delete or default
- if (Tok.is(tok::semi)) {
- ConsumeToken();
- }
+ if (Tok.is(tok::semi))
+ ConsumeExtraSemi(AfterDefinition, /*CanStartLine*/false);
return;
}
@@ -2262,10 +2261,8 @@ void Parser::ParseCXXMemberSpecification(SourceLocation RecordLoc,
// Check for extraneous top-level semicolon.
if (Tok.is(tok::semi)) {
- Diag(Tok, diag::ext_extra_struct_semi)
- << DeclSpec::getSpecifierName((DeclSpec::TST)TagType)
- << FixItHint::CreateRemoval(Tok.getLocation());
- ConsumeToken();
+ ConsumeExtraSemi(InsideStruct, /*CanStartLine*/true,
+ DeclSpec::getSpecifierName((DeclSpec::TST)TagType));
continue;
}
@@ -2935,10 +2932,8 @@ void Parser::ParseMicrosoftIfExistsClassDeclaration(DeclSpec::TST TagType,
// Check for extraneous top-level semicolon.
if (Tok.is(tok::semi)) {
- Diag(Tok, diag::ext_extra_struct_semi)
- << DeclSpec::getSpecifierName((DeclSpec::TST)TagType)
- << FixItHint::CreateRemoval(Tok.getLocation());
- ConsumeToken();
+ ConsumeExtraSemi(InsideStruct, /*CanStartLine*/true,
+ DeclSpec::getSpecifierName((DeclSpec::TST)TagType));
continue;
}
diff --git lib/Parse/ParseObjc.cpp lib/Parse/ParseObjc.cpp
index c664c64..95d74e2 100644
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -1257,9 +1257,7 @@ void Parser::ParseObjCClassInstanceVariables(Decl *interfaceDecl,
// Check for extraneous top-level semicolon.
if (Tok.is(tok::semi)) {
- Diag(Tok, diag::ext_extra_ivar_semi)
- << FixItHint::CreateRemoval(Tok.getLocation());
- ConsumeToken();
+ ConsumeExtraSemi(InstanceVariableList, /*CanStartLine*/true);
continue;
}
diff --git lib/Parse/Parser.cpp lib/Parse/Parser.cpp
index dd339f5..9fba829 100644
--- lib/Parse/Parser.cpp
+++ lib/Parse/Parser.cpp
@@ -201,6 +201,31 @@ bool Parser::ExpectAndConsumeSemi(unsigned DiagID) {
return ExpectAndConsume(tok::semi, DiagID);
}
+void Parser::ConsumeExtraSemi(ExtraSemiKind Kind, bool CanStartLine,
+ const char* DiagMsg) {
+ if (!Tok.is(tok::semi)) return;
+ if (Tok.isAtStartOfLine() && !CanStartLine) return;
+
+ SourceLocation StartLoc = Tok.getLocation();
+ SourceLocation EndLoc = Tok.getLocation();
+ ConsumeToken();
+
+ while ((Tok.is(tok::semi) && !Tok.isAtStartOfLine())) {
+ EndLoc = Tok.getLocation();
+ ConsumeToken();
+ }
+
+ if (Kind == OutsideFunction && getLangOpts().CPlusPlus0x) {
+ Diag(StartLoc, diag::warn_cxx98_compat_top_level_semi)
+ << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
+ return;
+ }
+
+ Diag(StartLoc, diag::ext_extra_semi)
+ << Kind << DiagMsg << FixItHint::CreateRemoval(SourceRange(StartLoc,
+ EndLoc));
+}
+
//===----------------------------------------------------------------------===//
// Error recovery.
//===----------------------------------------------------------------------===//
@@ -560,11 +585,7 @@ Parser::ParseExternalDeclaration(ParsedAttributesWithRange &attrs,
HandlePragmaPack();
return DeclGroupPtrTy();
case tok::semi:
- Diag(Tok, getLangOpts().CPlusPlus0x ?
- diag::warn_cxx98_compat_top_level_semi : diag::ext_top_level_semi)
- << FixItHint::CreateRemoval(Tok.getLocation());
-
- ConsumeToken();
+ ConsumeExtraSemi(OutsideFunction, /*CanStartLine=*/true);
// TODO: Invoke action for top-level semicolon.
return DeclGroupPtrTy();
case tok::r_brace:
diff --git test/Misc/warning-flags.c test/Misc/warning-flags.c
index bc0c941..c13d48c 100644
--- test/Misc/warning-flags.c
+++ test/Misc/warning-flags.c
@@ -17,7 +17,7 @@ This test serves two purposes:
The list of warnings below should NEVER grow. It should gradually shrink to 0.
-CHECK: Warnings without flags (253):
+CHECK: Warnings without flags (250):
CHECK-NEXT: ext_anonymous_struct_union_qualified
CHECK-NEXT: ext_binary_literal
CHECK-NEXT: ext_cast_fn_obj
@@ -33,8 +33,6 @@ CHECK-NEXT: ext_enumerator_list_comma
CHECK-NEXT: ext_expected_semi_decl_list
CHECK-NEXT: ext_explicit_instantiation_without_qualified_id
CHECK-NEXT: ext_explicit_specialization_storage_class
-CHECK-NEXT: ext_extra_ivar_semi
-CHECK-NEXT: ext_extra_struct_semi
CHECK-NEXT: ext_forward_ref_enum
CHECK-NEXT: ext_freestanding_complex
CHECK-NEXT: ext_hexconstant_invalid
@@ -65,7 +63,6 @@ CHECK-NEXT: ext_return_has_void_expr
CHECK-NEXT: ext_subscript_non_lvalue
CHECK-NEXT: ext_template_arg_extra_parens
CHECK-NEXT: ext_thread_before
-CHECK-NEXT: ext_top_level_semi
CHECK-NEXT: ext_typecheck_addrof_void
CHECK-NEXT: ext_typecheck_cast_nonscalar
CHECK-NEXT: ext_typecheck_cast_to_union
diff --git test/Parser/cxx-class.cpp test/Parser/cxx-class.cpp
index 36f08d5..01eb829 100644
--- test/Parser/cxx-class.cpp
+++ test/Parser/cxx-class.cpp
@@ -14,9 +14,9 @@ protected:
public:
void m() {
int l = 2;
- };
+ }; // expected-warning{{extra ';' after function definition}}
- template<typename T> void mt(T) { };
+ template<typename T> void mt(T) { }
; // expected-warning{{extra ';' inside a class}}
virtual int vf() const volatile = 0;
diff --git test/Parser/cxx-extra-semi.cpp test/Parser/cxx-extra-semi.cpp
new file mode 100644
index 0000000..9cded9c
--- /dev/null
+++ test/Parser/cxx-extra-semi.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi -Werror %t
+
+class A {
+ void A1();
+ void A2() { }; // expected-warning{{extra ';' after function definition}}
+ ; // expected-warning{{extra ';' inside a class}}
+ void A3() { }; ;; // expected-warning{{extra ';' after function definition}}
+ ;;;;;;; // expected-warning{{extra ';' inside a class}}
+ ; // expected-warning{{extra ';' inside a class}}
+ ; ;; ; ;;; // expected-warning{{extra ';' inside a class}}
+ ; ; ; ; ;; // expected-warning{{extra ';' inside a class}}
+ void A4();
+};
+
+union B {
+ int a1;
+ int a2;; // expected-warning{{extra ';' inside a union}}
+};
+
+; // expected-warning{{extra ';' outside of a function}}
+; ;;// expected-warning{{extra ';' outside of a function}}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
