On Sep 30, 2014, at 2:45 PM, Reid Kleckner <[email protected]> wrote: > +def CXX11WarnOverrideMethod : > DiagGroup<"warn-missing-override-on-overriding-method">; > > Anyone care to bikeshed the name a bit? This flag feels really verbose, > especially given that it gets a -W prefix. > > Can we go with -Wmissing-override or -Winconsistent-missing-override to > reflect the fact that it's heuristically triggered based on use of C++11 > override somewhere in the class hierarchy?
I selected your suggestion of -Winconsistent-missing-override. I think I have covered everything else and Doug responded to some of your comments let me know if I missed anything. In r218925. - Fariborz > > On Fri, Sep 26, 2014 at 4:15 PM, jahanian <[email protected]> wrote: > > 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 > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
