[Lldb-commits] [lldb] r222186 - Fix override/virtual warnings.
Vince Harron
vharron at google.com
Mon Nov 17 18:23:29 PST 2014
Hi Jim/Zach,
It seems like a really good idea to use the override keyword. IIUC it
basically lets you assert that there is a function with the same signature
in the base class that it needs to bind to.
I would guess that almost everyone has been bitten by lack of this assert
at some point (or multiple times) in the past.
On Mon, 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.
>
> 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
>>
>>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
--
Vince Harron | Technical Lead Manager | vharron at google.com | 858-442-0868
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20141117/100cbf33/attachment.html>
More information about the lldb-commits
mailing list