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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
