[lldb-dev] [Lldb-commits] [lldb] r222186 - Fix override/virtual warnings.
David Blaikie
dblaikie at gmail.com
Mon Nov 17 20:54:03 PST 2014
On Mon, Nov 17, 2014 at 8:43 PM, Eric Christopher <echristo at gmail.com>
wrote:
>
>
> On Mon Nov 17 2014 at 6:31:34 PM Zachary Turner <zturner at google.com>
> wrote:
>
>> +lldb-dev
>>
>> I'm actually ok just migrating towards override slowly (i.e. when you
>> touch a class that overrides some methods, add the override keyword), but
>> if people think it should be all or nothing (and if nobody else volunteers
>> to help with the "all" part), then I guess I'll do it.
>>
>> Eric, or anyone else: Is there a compiler flag that will warn or error if
>> a virtual method in a derived class overrides one in a base class but does
>> not use the override keyword? Otherwise it's going to be difficult to find
>> all the places that require changing.
>>
>>
> Not sure, it's not my specialty :)
>
> Dave?
>
No warning for all cases of override. There's a clang-tidy warning for it
or a clang warning for "you have at least one use of 'override' in this
class, and there are other places in the class you could be using it"
So if you've got a cmake build, you can use that to generate the necessary
compilation database to integrate with tooling to run clang-tidy over and
automatically apply all the fixes. (not sure if it goes and removes virtual
too, while you're there - maybe there's a separate clang-tidy diagnostic
for that you can use)
- David
>
> -eric
>
>
>>
>> On Mon Nov 17 2014 at 6:16:55 PM <jingham at apple.com> wrote:
>>
>>>
>>> > On Nov 17, 2014, at 5:54 PM, Zachary Turner <zturner at google.com>
>>> wrote:
>>> >
>>> > It's true you have to remember whether you're in the base class, but
>>> as you deduced, that's pretty much the point. FWIW I switched all of
>>> ProcessWindows over to using it just a day ago, because I found some dead
>>> code that occurred due to not using it, which this would have found.
>>> Apparently process plugins used to have plugin logging and plugin commands,
>>> because ProcessLinux and ProcessWindows had stub methods like
>>> EnablePluginLogging and ExecutePluginCommand, but these methods were
>>> nowhere to be found in base classes. So I guess at some point they were
>>> deleted, but since virtual both declares a new virtual method and overrides
>>> an existing one, this dead code was never detected.
>>> >
>>> > This isn't quite as serious as introducing an actual bug, but either
>>> way, we can all agree that dead code sucks.
>>> >
>>> > Since the virtual keyword is optional but harmless on an override, we
>>> could always adopt the policy of requiring the "virtual" keyword on an
>>> override, which should address your concern of having to look in two
>>> places. This way you can still simply look at the beginning to see if a
>>> method is virtual or not, and override is just a little syntatic sugar on
>>> the end to get the compiler to generate an error when you actually make an
>>> error.
>>>
>>> I guess override doesn't go in the same place as virtual because the
>>> C++11 folks didn't want to grab another keyword, so they had to stick it at
>>> the end of a declaration so they could tell what it meant? That's
>>> unfortunate since it means two tokens with very similar semantic meaning
>>> occur in totally different places in the declaration, but I guess we're
>>> stuck with that.
>>>
>>> If we are going to use it we should probably stick to one or the other.
>>> Otherwise it's just even more noise.
>>>
>>> On the topic of noise, I also kinda hate the notion of the noise making
>>> all our code consistent would introduce, but if we are going to use it we
>>> should do so consistently. I have a strong vote which is that whoever
>>> votes FOR this should take on making it consistent, and those who vote
>>> against it can politely thank them for their efforts ;-)
>>>
>>> Jim
>>>
>>>
>>> >
>>> > On Mon Nov 17 2014 at 5:36:22 PM <jingham at apple.com> wrote:
>>> > Override seems kind of gross to me. I can't use it on base class
>>> functions. If I just use override I get
>>> >
>>> > override.cpp:3:22: error: only virtual member functions can be marked
>>> 'override'
>>> > int DoSomething () override;
>>> >
>>> > and if I put both I get:
>>> >
>>> > override.cpp:3:15: error: 'DoSomething' marked 'override' but does not
>>> override any member functions
>>> > virtual int DoSomething () override;
>>> > ^
>>> > Now I have to remember whether I'm in the base class for this method
>>> or not, and I have to look in two places in the declaration (one after lots
>>> of noise) to see whether the method is actually virtual or not.
>>> >
>>> > It's kinda nice that it tells me if I got a method a little wrong, but
>>> I wonder whether that "at the time I change method names" convenience is
>>> worth the more common reading through code annoyance. I vote not to use
>>> it, though it's not a strong vote. If we are going to use it we should go
>>> through and make its use consistent otherwise we'll keep getting errors
>>> where people copy from the base class and forget to switch all the
>>> "virtual" on the LHS to "override" on the RHS...
>>>
>>>
>>>
>>> >
>>> > Jim
>>> >
>>> >
>>> > > On Nov 17, 2014, at 5:28 PM, Zachary Turner <zturner at google.com>
>>> wrote:
>>> > >
>>> > > override implies virtual and is a strict superset of virtual, so
>>> there's nothing inherently wrong with "mixing" them. The advantage of
>>> override is that it will cause the compiler to error if, for example, you
>>> delete a virtual method from a base class (or change the signature), but
>>> forget to make a corresponding change in derived implementations.
>>> > >
>>> > > For example, if you have
>>> > >
>>> > > class A
>>> > > {
>>> > > public:
>>> > > virtual void Foo();
>>> > > };
>>> > >
>>> > > class B : public A
>>> > > {
>>> > > public:
>>> > > virtual void Foo();
>>> > > };
>>> > >
>>> > > this is all fine. If someone goes and changes A to the following:
>>> > >
>>> > > class A
>>> > > {
>>> > > public:
>>> > > virtual void Foo(int x);
>>> > > };
>>> > >
>>> > > This will continue to compile no problem, and not alert the user to
>>> the fact that there is now a bug in the code. On the other hand, if the
>>> original implementation of B had been like this:
>>> > >
>>> > > class B : public A
>>> > > {
>>> > > public:
>>> > > virtual void Foo() override; // keyword "virtual" is optional
>>> here
>>> > > };
>>> > >
>>> > > then when the change was made to A, B would generate a compiler
>>> error that there was no appropriate method to override.
>>> > >
>>> > > As far as deciding to switch to override, I propose we decide that
>>> right now :) There's no advantage to not doing so, since it is a strict
>>> superset of virtual, and surfaces a common source of programmer error as a
>>> compiler error.
>>> > >
>>> > > On Mon Nov 17 2014 at 4:43:31 PM <jingham at apple.com> wrote:
>>> > > As I understand it, mixing override & virtual is a recipe for
>>> confusion... We currently use virtual, and not override. At some point we
>>> can talk about switching over to using override everywhere, but I don't
>>> think we are at that point.
>>> > >
>>> > > So in this case your patch is incorrect, and the "virtual" should go
>>> back in. And then until we decide to switch to using override, can you
>>> turn off whatever warning is causing you to (as I understand it)
>>> incorrectly remove these "virtual" markers?
>>> > >
>>> > > Jim
>>> > >
>>> > >
>>> > > > On Nov 17, 2014, at 3:14 PM, Eric Christopher <echristo at gmail.com>
>>> wrote:
>>> > > >
>>> > > > It's fine as virtual, but you're overriding a method and not
>>> marking it as override. You don't need to write both down (you could, but
>>> it's a nop) as override only works on virtual functions.
>>> > > >
>>> > > > -eric
>>> > > >
>>> > > > On Mon Nov 17 2014 at 3:07:27 PM <jingham at apple.com> wrote:
>>> > > > I'm sure I'm missing something obvious, but I don't understand
>>> what this warning you are fixing is supposed to be telling you about. The
>>> inheritance hierarchy is:
>>> > > >
>>> > > > PlatformDarwin<--PlatformPOSIX<--Platform
>>> > > >
>>> > > > Platform.h defines:
>>> > > >
>>> > > > virtual Error
>>> > > > ResolveExecutable (const FileSpec &exe_file,
>>> > > > const ArchSpec &arch,
>>> > > > lldb::ModuleSP &module_sp,
>>> > > > const FileSpecList
>>> *module_search_paths_ptr);
>>> > > >
>>> > > >
>>> > > > Then PlatformPosix.h doesn't override ResolveExecutable, and
>>> finally, PlatformDarwin has:
>>> > > >
>>> > > > virtual lldb_private::Error
>>> > > > ResolveExecutable (const lldb_private::FileSpec &exe_file,
>>> > > > const lldb_private::ArchSpec &arch,
>>> > > > lldb::ModuleSP &module_sp,
>>> > > > const lldb_private::FileSpecList
>>> *module_search_paths_ptr);
>>> > > >
>>> > > >
>>> > > > Why is it wrong for PlatformDarwin to define this method as
>>> virtual?
>>> > > >
>>> > > > Jim
>>> > > >
>>> > > >
>>> > > >
>>> > > > > On Nov 17, 2014, at 2:55 PM, Eric Christopher <
>>> echristo at gmail.com> wrote:
>>> > > > >
>>> > > > > Author: echristo
>>> > > > > Date: Mon Nov 17 16:55:13 2014
>>> > > > > New Revision: 222186
>>> > > > >
>>> > > > > URL: http://llvm.org/viewvc/llvm-project?rev=222186&view=rev
>>> > > > > Log:
>>> > > > > Fix override/virtual warnings.
>>> > > > >
>>> > > > > Modified:
>>> > > > > lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwin.h
>>> > > > >
>>> > > > > Modified: lldb/trunk/source/Plugins/Platform/MacOSX/
>>> PlatformDarwin.h
>>> > > > > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/
>>> Plugins/Platform/MacOSX/PlatformDarwin.h?rev=222186&
>>> r1=222185&r2=222186&view=diff
>>> > > > > ============================================================
>>> ==================
>>> > > > > --- lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwin.h
>>> (original)
>>> > > > > +++ lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwin.h
>>> Mon Nov 17 16:55:13 2014
>>> > > > > @@ -27,57 +27,58 @@ public:
>>> > > > > //----------------------------------------------------------
>>> --
>>> > > > > // lldb_private::Platform functions
>>> > > > > //----------------------------------------------------------
>>> --
>>> > > > > - virtual lldb_private::Error
>>> > > > > + lldb_private::Error
>>> > > > > ResolveExecutable (const lldb_private::ModuleSpec
>>> &module_spec,
>>> > > > > lldb::ModuleSP &module_sp,
>>> > > > > const lldb_private::FileSpecList
>>> *module_search_paths_ptr) override;
>>> > > > >
>>> > > > > - virtual lldb_private::Error
>>> > > > > + lldb_private::Error
>>> > > > > ResolveSymbolFile (lldb_private::Target &target,
>>> > > > > const lldb_private::ModuleSpec &sym_spec,
>>> > > > > - lldb_private::FileSpec &sym_file);
>>> > > > > + lldb_private::FileSpec &sym_file)
>>> override;
>>> > > > >
>>> > > > > lldb_private::FileSpecList
>>> > > > > LocateExecutableScriptingResources (lldb_private::Target
>>> *target,
>>> > > > > lldb_private::Module
>>> &module,
>>> > > > > - lldb_private::Stream*
>>> feedback_stream);
>>> > > > > + lldb_private::Stream*
>>> feedback_stream) override;
>>> > > > >
>>> > > > > - virtual lldb_private::Error
>>> > > > > + lldb_private::Error
>>> > > > > GetSharedModule (const lldb_private::ModuleSpec &module_spec,
>>> > > > > lldb::ModuleSP &module_sp,
>>> > > > > const lldb_private::FileSpecList
>>> *module_search_paths_ptr,
>>> > > > > lldb::ModuleSP *old_module_sp_ptr,
>>> > > > > - bool *did_create_ptr);
>>> > > > > + bool *did_create_ptr) override;
>>> > > > >
>>> > > > > - virtual size_t
>>> > > > > + size_t
>>> > > > > GetSoftwareBreakpointTrapOpcode (lldb_private::Target
>>> &target,
>>> > > > > -
>>> lldb_private::BreakpointSite *bp_site);
>>> > > > > +
>>> lldb_private::BreakpointSite *bp_site) override;
>>> > > > >
>>> > > > > - virtual bool
>>> > > > > + bool
>>> > > > > GetProcessInfo (lldb::pid_t pid,
>>> > > > > - lldb_private::ProcessInstanceInfo
>>> &proc_info);
>>> > > > > + lldb_private::ProcessInstanceInfo
>>> &proc_info) override;
>>> > > > >
>>> > > > > - virtual lldb::BreakpointSP
>>> > > > > - SetThreadCreationBreakpoint (lldb_private::Target &target);
>>> > > > > + lldb::BreakpointSP
>>> > > > > + SetThreadCreationBreakpoint (lldb_private::Target &target)
>>> override;
>>> > > > >
>>> > > > > - virtual uint32_t
>>> > > > > + uint32_t
>>> > > > > FindProcesses (const lldb_private::ProcessInstanceInfoMatch
>>> &match_info,
>>> > > > > - lldb_private::ProcessInstanceInfoList
>>> &process_infos);
>>> > > > > -
>>> > > > > - virtual bool
>>> > > > > - ModuleIsExcludedForNonModuleSpecificSearches
>>> (lldb_private::Target &target, const lldb::ModuleSP &module_sp);
>>> > > > > -
>>> > > > > + lldb_private::ProcessInstanceInfoList
>>> &process_infos) override;
>>> > > > > +
>>> > > > > + bool
>>> > > > > + ModuleIsExcludedForNonModuleSpecificSearches(lldb_private::Target
>>> &target,
>>> > > > > + const
>>> lldb::ModuleSP &module_sp) override;
>>> > > > > +
>>> > > > > bool
>>> > > > > ARMGetSupportedArchitectureAtIndex (uint32_t idx,
>>> lldb_private::ArchSpec &arch);
>>> > > > >
>>> > > > > bool
>>> > > > > x86GetSupportedArchitectureAtIndex (uint32_t idx,
>>> lldb_private::ArchSpec &arch);
>>> > > > >
>>> > > > > - virtual int32_t
>>> > > > > - GetResumeCountForLaunchInfo (lldb_private::ProcessLaunchInfo
>>> &launch_info);
>>> > > > > + int32_t
>>> > > > > + GetResumeCountForLaunchInfo (lldb_private::ProcessLaunchInfo
>>> &launch_info) override;
>>> > > > >
>>> > > > > - virtual void
>>> > > > > - CalculateTrapHandlerSymbolNames ();
>>> > > > > + void
>>> > > > > + CalculateTrapHandlerSymbolNames () override;
>>> > > > >
>>> > > > > protected:
>>> > > > >
>>> > > > >
>>> > > > >
>>> > > > > _______________________________________________
>>> > > > > lldb-commits mailing list
>>> > > > > lldb-commits at cs.uiuc.edu
>>> > > > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>> > > >
>>> > >
>>> > >
>>> > > _______________________________________________
>>> > > lldb-commits mailing list
>>> > > lldb-commits at cs.uiuc.edu
>>> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20141117/4165f904/attachment.html>
More information about the lldb-dev
mailing list