[lldb-dev] [Lldb-commits] [lldb] r222186 - Fix override/virtual warnings.
Eric Christopher
echristo at gmail.com
Mon Nov 17 20:43:33 PST 2014
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?
-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/20141118/b100ebea/attachment.html>
More information about the lldb-dev
mailing list