On Fri, Aug 2, 2013 at 2:34 PM, Arthur O'Dwyer <[email protected]> wrote: > On Fri, Aug 2, 2013 at 2:25 PM, David Blaikie <[email protected]> wrote: >> ================ >> Comment at: lib/Sema/SemaDeclCXX.cpp:4248 >> @@ +4247,3 @@ >> + // Also avoid issuing this diagnostic if not in C++11 mode. >> + if (getLangOpts().CPlusPlus11 && Record->hasOverrideAnnotation()) >> + CheckMissingOverrideSpecifier(*this, *M); >> ---------------- >> Presumably we want to issue this in C++14, etc. So perhaps "not C++98"? > > It shouldn't even be that; it should be "nothing". In C++98/03, the > hasOverrideAnnotation() method already returns false, so the > getLangOpts() check is just slowing things down unnecessarily.
Actually it's the opposite - it's very cheap to check the language level & avoids the expense of scanning all the members when we're in a language where we'll never find those attributes. Though, you're right in the sense that it's not necessary, just a (possibly premature) optimization. > Besides > which, we want to allow for the possibility that someone will > eventually get around to implementing __attribute__((override)) for > C++98 mode. Sure, and at that point we could generalize this diagnostic to include that. > >> Here are some thoughts (possibly wrong): >> >> 1) should we let 'final' imply 'override' (I know it's not necessarily true, >> 'final' could be placed at the >> start of an inheritance hierarchy) > > Right; so one doesn't imply the other (in either direction). To give more detail. Yes, I know it doesn't actually imply either way, but I wonder whether users will find it helpful or just an annoyance to be encouraged/forced to annotate final functions with override too. I don't have a strong feeling either way. >> 2) should we use the presence of 'final' to imply that the user wanted to >> use explicit 'override' >> (maybe not, I suppose - someone could be using one feature without the other >> consistently) > > You mean should Clang give a diagnostic for "final" without > "override", and/or "final" on a non-overriding declaration? The code as-is obviously provides a warning for final without override on an override. My question in (1) was whether we should do that or not. In (2) I was asking whether we should use the presence of a 'final' in the class as an indicator that we should warn about missing 'override' (on other methods - as we already (in this patch) use the presence of one 'override' to imply that we should warn about other, missing, 'override's). > Sounds > like a desirable feature to me, but in my experience this sort of > style guideline is always handled by saying "Style isn't upstream's > job; write a Clang plugin if you want to enforce that style on your > own codebase." Which I think would be the appropriate response in this > case, too. > > –Arthur > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
