On Tue, Apr 3, 2012 at 3:52 PM, Richard Trieu <[email protected]> wrote:
> On Mon, Mar 26, 2012 at 4:44 PM, David Blaikie <[email protected]> wrote: > >> On Mon, Mar 26, 2012 at 2:43 PM, Richard Trieu <[email protected]> wrote: >> > On Tue, Feb 14, 2012 at 11:10 AM, Richard Trieu <[email protected]> >> wrote: >> >> >> >> On Mon, Feb 13, 2012 at 4:05 PM, David Blaikie <[email protected]> >> wrote: >> >>> >> >>> On Mon, Feb 13, 2012 at 3:58 PM, Richard Trieu <[email protected]> >> wrote: >> >>> > On Mon, Feb 13, 2012 at 3:13 PM, David Blaikie <[email protected]> >> >>> > wrote: >> >>> >> >> >>> >> On Mon, Feb 13, 2012 at 2:37 PM, Richard Trieu <[email protected]> >> >>> >> wrote: >> >>> >> > On Mon, Feb 13, 2012 at 1:59 PM, David Blaikie < >> [email protected]> >> >>> >> > wrote: >> >>> >> >> >> >>> >> >> On Mon, Feb 13, 2012 at 1:52 PM, Richard Trieu < >> [email protected]> >> >>> >> >> wrote: >> >>> >> >> > On Mon, Jan 30, 2012 at 10:40 AM, Richard Trieu >> >>> >> >> > <[email protected]> >> >>> >> >> > wrote: >> >>> >> >> >> >> >>> >> >> >> On Wed, Jan 25, 2012 at 5:24 PM, Richard Trieu >> >>> >> >> >> <[email protected]> >> >>> >> >> >> 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.
extra-semi3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
