<div class="gmail_quote">On Tue, Apr 3, 2012 at 3:52 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com">rtrieu@google.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="gmail_quote"><div><div class="h5">On Mon, Mar 26, 2012 at 4:44 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">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><div>On Mon, Mar 26, 2012 at 2:43 PM, Richard Trieu <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>> wrote:<br>
> On Tue, Feb 14, 2012 at 11:10 AM, Richard Trieu <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>> wrote:<br>
>><br>
>> On Mon, Feb 13, 2012 at 4:05 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>> On Mon, Feb 13, 2012 at 3:58 PM, Richard Trieu <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>> wrote:<br>
>>> > On Mon, Feb 13, 2012 at 3:13 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
>>> > wrote:<br>
>>> >><br>
>>> >> On Mon, Feb 13, 2012 at 2:37 PM, Richard Trieu <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>><br>
>>> >> wrote:<br>
>>> >> > On Mon, Feb 13, 2012 at 1:59 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">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" target="_blank">rtrieu@google.com</a>><br>
>>> >> >> wrote:<br>
>>> >> >> > On Mon, Jan 30, 2012 at 10:40 AM, Richard Trieu<br>
>>> >> >> > <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>><br>
>>> >> >> > wrote:<br>
>>> >> >> >><br>
>>> >> >> >> On Wed, Jan 25, 2012 at 5:24 PM, Richard Trieu<br>
>>> >> >> >> <<a href="mailto:rtrieu@google.com" target="_blank">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<br>
>>> >> >> >>> is<br>
>>> >> >> >>> to<br>
>>> >> >> >>> make<br>
>>> >> >> >>> it easier to see which scopes are being ended by a brace.<br>
>>> >> >> >>> Function<br>
>>> >> >> >>> scopes<br>
>>> >> >> >>> will end with a plain brace. Class scopes will end with brace<br>
>>> >> >> >>> and<br>
>>> >> >> >>> semi-colon.<br>
>>> >> >> >>><br>
>>> >> >> >>> class C {<br>
>>> >> >> >>> void F() {};<br>
>>> >> >> >>> };<br>
>>> >> >> >>><br>
>>> >> >> >>> extra-semi.cc:2:14: warning: extra ';' after function<br>
>>> >> >> >>> 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<br>
>>> >> >> 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<br>
>>> >> > 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<br>
>>> >> > 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<br>
>>> > token<br>
>>> > and add this check there, mainly so the diagnostic has enough context<br>
>>> > to<br>
>>> > emit the right error message.<br>
>>><br>
>>> 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>
>><br>
>><br>
>> Took some of David's suggestions and modified the patch. Condensed all<br>
>> the extra semi warnings into a single diagnostic message with a %select and<br>
>> with the flag -Wextra-semi. C++98 compat warning was kept separate. Moved<br>
>> the emitting of logic and semi consumption to a shared function. I also<br>
>> added logic so that lines of semi-colons now generate a single warning<br>
>> instead of a warning per semi-colon.<br>
><br>
><br>
> Ping.<br>
<br>
</div></div>Attached an updated patch for ToT.<br>
<br>
This mostly looks fine to me. Though I don't quite understand the<br>
"CanStartLine" parameter - is this just to differentiate the<br>
diagnostic for:<br>
<br>
void func() {<br>
};<br>
<br>
and:<br>
<br>
void func() {<br>
}<br>
;<br>
<br>
? or is there some other reason for that parameter?</blockquote><div><br></div></div></div><div>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.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> When I removed the<br>
check in ConsumeExtraSemi it only caused the Parser/cxx-class.cpp to<br>
fail. (I'm not sure what cxx-class.cpp is meant to test - it seems<br>
like a sort of random grab-bag but perhaps so long as it exists it's<br>
an OK place for you to test those two cases)<br>
<br>
[Personally I wouldn't bother differentiating these diagnostics at all<br>
(drop the select, drop the extra contextual parameters to<br>
ConsumeExtraSemi (well I suppose you need the OutsideFunction state<br>
specifically to catch the compat warning)) but if you/others think<br>
they're worthwhile I'm OK with that too. Just my 2c there.]<br>
</blockquote></div></div><br><div>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.</div>
</blockquote></div>Ping. With updated patch.