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.
Index: test/Misc/warning-flags.c
===================================================================
--- test/Misc/warning-flags.c (revision 148994)
+++ test/Misc/warning-flags.c (working copy)
@@ -17,7 +17,7 @@
The list of warnings below should NEVER grow. It should gradually shrink to 0.
-CHECK: Warnings without flags (266):
+CHECK: Warnings without flags (263):
CHECK-NEXT: ext_anon_param_requires_type_specifier
CHECK-NEXT: ext_anonymous_struct_union_qualified
CHECK-NEXT: ext_array_init_copy
@@ -35,8 +35,6 @@
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
@@ -68,7 +66,6 @@
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
Index: test/Parser/cxx-extra-semi.cpp
===================================================================
--- test/Parser/cxx-extra-semi.cpp (revision 0)
+++ test/Parser/cxx-extra-semi.cpp (revision 0)
@@ -0,0 +1,25 @@
+// 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}}
+
Index: test/Parser/cxx-class.cpp
===================================================================
--- test/Parser/cxx-class.cpp (revision 148994)
+++ test/Parser/cxx-class.cpp (working copy)
@@ -14,9 +14,9 @@
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;
Index: include/clang/Basic/DiagnosticParseKinds.td
===================================================================
--- include/clang/Basic/DiagnosticParseKinds.td (revision 148994)
+++ include/clang/Basic/DiagnosticParseKinds.td (working copy)
@@ -21,15 +21,16 @@
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<
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h (revision 148994)
+++ include/clang/Parse/Parser.h (working copy)
@@ -649,6 +649,21 @@
/// 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
Index: lib/Parse/ParseDecl.cpp
===================================================================
--- lib/Parse/ParseDecl.cpp (revision 148994)
+++ lib/Parse/ParseDecl.cpp (working copy)
@@ -2750,10 +2750,8 @@
// 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;
}
Index: lib/Parse/ParseObjc.cpp
===================================================================
--- lib/Parse/ParseObjc.cpp (revision 148994)
+++ lib/Parse/ParseObjc.cpp (working copy)
@@ -1250,9 +1250,7 @@
// 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;
}
Index: lib/Parse/ParseDeclCXX.cpp
===================================================================
--- lib/Parse/ParseDeclCXX.cpp (revision 148994)
+++ lib/Parse/ParseDeclCXX.cpp (working copy)
@@ -1884,9 +1884,8 @@
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;
}
@@ -2229,10 +2228,8 @@
// 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;
}
@@ -2899,10 +2896,8 @@
// 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;
}
Index: lib/Parse/Parser.cpp
===================================================================
--- lib/Parse/Parser.cpp (revision 148994)
+++ lib/Parse/Parser.cpp (working copy)
@@ -198,6 +198,31 @@
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 && getLang().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.
//===----------------------------------------------------------------------===//
@@ -544,11 +569,7 @@
Decl *SingleDecl = 0;
switch (Tok.getKind()) {
case tok::semi:
- Diag(Tok, getLang().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:
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
