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

[Richard Trieu]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120214/1e6042ad/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extra-semi2.patch
Type: text/x-patch
Size: 9212 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120214/1e6042ad/attachment.bin>