[Lldb-commits] [lldb] r222186 - Fix override/virtual warnings.
Zachary Turner
zturner at google.com
Mon Nov 17 17:28:00 PST 2014
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-commits/attachments/20141118/98d29998/attachment.html>
More information about the lldb-commits
mailing list