<div class="gmail_quote">On Mon, Feb 13, 2012 at 4:05 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Mon, Feb 13, 2012 at 3:58 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
> On Mon, Feb 13, 2012 at 3:13 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, Feb 13, 2012 at 2:37 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>> > On Mon, Feb 13, 2012 at 1:59 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Mon, Feb 13, 2012 at 1:52 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>><br>
>> >> wrote:<br>
>> >> > On Mon, Jan 30, 2012 at 10:40 AM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> On Wed, Jan 25, 2012 at 5:24 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>><br>
>> >> >> wrote:<br>
>> >> >>><br>
>> >> >>> This patch adds a warning to catch semi-colons after function<br>
>> >> >>> definitions. The motivation of removing extraneous semi-colons is<br>
>> >> >>> to<br>
>> >> >>> make<br>
>> >> >>> it easier to see which scopes are being ended by a brace. Function<br>
>> >> >>> scopes<br>
>> >> >>> will end with a plain brace. Class scopes will end with brace and<br>
>> >> >>> semi-colon.<br>
>> >> >>><br>
>> >> >>> class C {<br>
>> >> >>> void F() {};<br>
>> >> >>> };<br>
>> >> >>><br>
>> >> >>> extra-semi.cc:2:14: warning: extra ';' after function definition<br>
>> >> >>> [-Wextra-semi]<br>
>> >> >>> void F() {};<br>
>> >> >>> ^<br>
>> >> >>> 1 warning generated.<br>
>> >> >><br>
>> >> >><br>
>> >> >> Ping.<br>
>> >> ><br>
>> >> > Ping.<br>
>> >><br>
>> >> Not to derail this review - but just out of curiosity: do we have a<br>
>> >> more general 'unnecessary ;' warning? or could we make a general one<br>
>> >> that covers all cases where ';' is just redudndant & suggests a<br>
>> >> removal?<br>
>> ><br>
>> > There's a few in the parse diagnostics. They are:<br>
>> ><br>
>> > ext_top_level_semi<br>
>> > warn_cxx98_compat_top_level_semi<br>
>> > ext_extra_struct_semi<br>
>> > ext_extra_ivar_semi<br>
>> ><br>
>> > warn_cxx98_compat_top_level_semi is a warning that is in a diagnostic<br>
>> > group<br>
>> > so it must be left separate. The other 3 are Extensions and could<br>
>> > probably<br>
>> > be folded in with my diagnostic if I switched mine to Extension as well<br>
>> > and<br>
>> > added a select into the message.<br>
>><br>
>><br>
>> Oh, sorry - I wasn't so much concerned with the diagnostic text, as<br>
>> the logic - is there some way we could, in a relatively<br>
>> universal/general way, catch all/most unnecessary semicolons rather<br>
>> than adding it on a case-by-case basis like this?<br>
><br>
><br>
> Yup, that's also possible. It's usually a simple check like:<br>
><br>
> if (Tok.is(tok::semi)) {<br>
> // Emit Diagnostic<br>
> ConsumeToken();<br>
> }<br>
><br>
> And replace it with a ConsumeExtraSemi function call with a partial<br>
> diagnostic argument. Then we can add some more robust logic into semi<br>
> checking. But we'll still need to find every check for a semi-colon token<br>
> and add this check there, mainly so the diagnostic has enough context to<br>
> emit the right error message.<br>
<br>
</div></div>Fair enough - though I'm not sure if a generic error message would be<br>
terribly problematic (do you really need to know that this particular<br>
spurious semi is after a member function definition rather than at the<br>
global scope?)<br>
<br>
If the checks are that regular it'd probably make sense to add a<br>
utility function to sema (similar to the brace handling, etc) to<br>
conusme - with an optional diagnostic to emit & a default to use<br>
otherwise. But yeah, I suppose the essential answer to my question, as<br>
it pertains to this CR is "no, there's not one place we can fix this<br>
at the moment - but a pretty common cliche we can sweep to cleanup if<br>
we want to spend the tiime at some ponit"<br>
<br>
Thanks,<br>
- David<br>
</blockquote></div><br><div>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.</div>