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

[Richard Trieu]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120326/2ab0b4c5/attachment.html>