This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Convert the default constructor’s member initializers into default member initializers
ClosedPublic

Authored by JDevlieghere on Jun 1 2021, 12:16 PM.

Details

Summary

Converts a default constructor’s member initializers into the new default member initializers in C++11. Other member initializers that match the default member initializer are removed.

This was automatically generated with modernize-use-default-member-init and our .clang-tidy file was updated accordingly.

run-clang-tidy.py -header-filter='lldb' -checks='-*,modernize-use-default-member-init' -fix

This is a mass-refactoring patch and the resulting commit should be added to .git-blame-ignore-revs.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jun 1 2021, 12:16 PM
JDevlieghere requested review of this revision.Jun 1 2021, 12:16 PM

FWIW this is more of an RFC and as always there's some manual fixing up to be done and formatting to be fixed. I wanted to put this up for review before sinking time in that.

+1. I'm anyway doing the same whenever I have to touching constructors, so we might as well pull out the big hammer.

I'm also glad you volunteer to resolve all the downstream conflicts :)

lldb/include/lldb/Breakpoint/BreakpointOptions.h
46

If we anyway touch this, could we also delete these default member initializers?

jingham added a subscriber: jingham.Jun 1 2021, 3:22 PM

Thanks for taking this on. The in-class initialization is so much less boiler-plate and easier to read!

shafik added a comment.Jun 2 2021, 2:45 PM

Thank you for doing this! This will be a big improvement.

I am not done going through this change but I think it will require a bit more careful look though to make sure we are getting the maximum benefit from this refactor.

lldb/include/lldb/Utility/VMRange.h
29

= default

lldb/source/API/SBBroadcaster.cpp
19

We don't need m_opaque_sp() this is a std::shared_ptr the default constructor will do the right thing.

24

We don't need m_opaque_ptr(nullptr)

lldb/source/API/SBCommandReturnObject.cpp
24

We can remove m_ptr(new CommandReturnObject(false)) and use =default

43

CommandReturnObject *m_ptr = new CommandReturnObject(false)

Note, this is ok w/ the other constructors b/c class.base.init/p10 tell us

If a given non-static data member has both a default member initializer and a mem-initializer, the initialization specified by the mem-initializer is performed, and the non- static data member's default member initializer is ignored.

aprantl accepted this revision.Jun 2 2021, 3:31 PM

Impressive. I spot-checked a few instances and this generally looks great. Let's hope the test suite agrees!

This revision is now accepted and ready to land.Jun 2 2021, 3:31 PM
aprantl added inline comments.Jun 2 2021, 3:33 PM
lldb/include/lldb/Host/FileSystem.h
35

why is m_collector not caught? Because it's also explicitly initialized below?

36

but then — why isn't m_home_directory() removed?

Fix compilation issues

teemperor accepted this revision.Jun 9 2021, 8:23 AM

This doesn't compile for me (on Linux):

In file included from /home/teemperor/work/ci/llvm-project/lldb/unittests/Platform/PlatformAppleSimulatorTest.cpp:11:
In file included from /home/teemperor/work/ci/llvm-project/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h:15:
/home/teemperor/work/ci/llvm-project/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h:96:14: error: use of undeclared identifier 'nil'
  id m_dev = nil;
             ^
/home/teemperor/work/ci/llvm-project/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h:132:14: error: use of undeclared identifier 'nil'
  id m_dev = nil;
             ^
/home/teemperor/work/ci/llvm-project/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h:172:14: error: use of undeclared identifier 'nil'
  id m_dev = nil;
             ^

The nil is some Obj-C thing but the headers are includes from a C++ file. I don't think we should add Obj-C includes to the header as we really need to keep the Objective-C++ code in LLDB as small as possible, so maybe change this to nullptr?

Beside that I couldn't find anything that would block this patch. There are a bunch of additional NFC cleanups possible but that seems out of scope, so LGTM modulo nil -> nullptr.

  • Rebase
  • Replace nil with nullptr
This revision was landed with ongoing or failed builds.Jun 9 2021, 9:43 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2021, 9:43 AM