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

Tue Apr 24 16:08:42 PDT 2012

On Tue, Apr 3, 2012 at 3:52 PM, Richard Trieu <rtrieu at google.com> wrote:

> On Mon, Mar 26, 2012 at 4:44 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> 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?
>
>
> Yes, that's exactly the two cases that the parameter differentiates.  The
> first gives an extra semi after a function definition warning while the
> second gives an extra semi outside of a function warning.
>
>
>> 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.]
>>
>
> I think providing more information is preferable to less information in a
> diagnostic, especially since the extra information in this warning will
> still keep it under a line long.
>
Ping.  With updated patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120424/46325815/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extra-semi3.patch
Type: application/octet-stream
Size: 9198 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120424/46325815/attachment.obj>