[cfe-commits] [Patch] Add warning for semi-colon after class function definition

[David Blaikie]

On Mon, Mar 26, 2012 at 2:43 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Tue, Feb 14, 2012 at 11:10 AM, Richard Trieu <rtrieu at google.com> wrote:
>>
>> On Mon, Feb 13, 2012 at 4:05 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> On Mon, Feb 13, 2012 at 3:58 PM, Richard Trieu <rtrieu at google.com> wrote:
>>> > On Mon, Feb 13, 2012 at 3:13 PM, David Blaikie <dblaikie at gmail.com>
>>> > wrote:
>>> >>
>>> >> On Mon, Feb 13, 2012 at 2:37 PM, Richard Trieu <rtrieu at google.com>
>>> >> wrote:
>>> >> > On Mon, Feb 13, 2012 at 1:59 PM, David Blaikie <dblaikie at gmail.com>
>>> >> > wrote:
>>> >> >>
>>> >> >> On Mon, Feb 13, 2012 at 1:52 PM, Richard Trieu <rtrieu at google.com>
>>> >> >> wrote:
>>> >> >> > On Mon, Jan 30, 2012 at 10:40 AM, Richard Trieu
>>> >> >> > <rtrieu at google.com>
>>> >> >> > wrote:
>>> >> >> >>
>>> >> >> >> On Wed, Jan 25, 2012 at 5:24 PM, Richard Trieu
>>> >> >> >> <rtrieu at google.com>
>>> >> >> >> 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.]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extra-semi.patch
Type: text/x-patch
Size: 9464 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120326/00f67d6e/attachment.bin>