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

Tue Apr 3 15:52:33 PDT 2012

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