[Lldb-commits] [lldb] r222186 - Fix override/virtual warnings.
Zachary Turner
zturner at google.com
Mon Nov 17 21:29:36 PST 2014
Maybe the simplest thing to do is find the code for the "you have at least
one use of override", change 1 to 0, and then use that version of clang :)
On Mon Nov 17 2014 at 8:54:04 PM David Blaikie <dblaikie at gmail.com> wrote:
> 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
>>>> > > > > + ModuleIsExcludedForNonModuleSp
>>>> ecificSearches(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/338670dd/attachment.html>
More information about the lldb-commits
mailing list