On Sep 26, 2014, at 4:12 PM, Douglas Gregor <[email protected]> wrote:
> LGTM > Thanks. I want to add FixIts before checking it in. - Fariborz >> On Sep 26, 2014, at 4:10 PM, jahanian <[email protected]> wrote: >> >> >> On Sep 26, 2014, at 3:37 PM, Douglas Gregor <[email protected]> wrote: >> >>> >>>> On Sep 26, 2014, at 3:03 PM, jahanian <[email protected]> wrote: >>>> >>>> >>>> On Sep 25, 2014, at 11:51 AM, Argyrios Kyrtzidis <[email protected]> >>>> wrote: >>>> >>>>> >>>>>> On Sep 25, 2014, at 11:24 AM, Douglas Gregor <[email protected]> wrote: >>>>>> >>>>>> I’d feel a lot better if some part of the warning could be on by >>>>>> default. For example, if you’ve uttered “override” at least once in a >>>>>> class, it makes sense to warn-by-default about any other overrides in >>>>>> that class that weren’t marked as “override”, because you’re being >>>>>> locally inconsistent. Or maybe you can expand that heuristic out to a >>>>>> file-level granularity (which matches better for the null point constant >>>>>> warning) and still be on-by-default. >>>>> >>>>> This seems like a great idea to me! >>>>> For the 'override' I much prefer if it is class specific to make it less >>>>> of a burden as an “always on” warning. We could have the checking done at >>>>> the end of the class definition. >>>>> >>>> >>>> Here is the patch. Warning is on by default. Number of new warnings on >>>> clang tests is greatly reduced but there are still some. >>> >>> +def warn_function_marked_not_override_overriding : Warning < >>> + "%0 is not marked 'override' but overrides a member functions">, >>> + InGroup<CXX11WarnOverrideMethod>; >>> >>> “a member functions” shouldn’t be plural. Also, perhaps we should turn this >>> around: >>> >>> “%0 overrides a member function but is not marked ‘override’” >>> >>> because that puts the context of the problem before the problem. >>> >>> + if (HasMethodWithOverrideControl) { >>> + // At list one method has the 'override' control declared. >>> + // Diagnose all other overridden methods which do not have 'override' >>> specified on them. >>> + for (auto *M : Record->methods()) >>> >>> “At list” -> “At least”. >>> >>> Also, this means we’ll be taking two passes over the methods if any >>> “override” is present, even though we won’t often warn here. How about >>> extending this: >>> >>> + if (M->hasAttr<OverrideAttr>()) >>> + HasMethodWithOverrideControl = true; >>> >>> with >>> >>> else if (M->begin_overridden_methods() != M->end_overridden_methods()) >>> HasOverridingMethodWithoutOverrideControl = true; >>> >>> and we only do this second pass when we know we’re going to warn, e.g., if >>> HasMethodWithOverrideControl && HasOverridingMethodWithoutOverrideControl? >> >> Thanks for quick review. Here is the updated patch. >> >> <override-patch.txt> >> >> - Fariborz >>> >>> - Doug >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
