[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

2021-06-09 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor accepted this revision.
teemperor added a comment.

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`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103483/new/

https://reviews.llvm.org/D103483

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

2021-06-02 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added inline comments.



Comment at: lldb/include/lldb/Host/FileSystem.h:35
   FileSystem()
   : m_fs(llvm::vfs::getRealFileSystem()), m_collector(nullptr),
+m_home_directory() {}

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



Comment at: lldb/include/lldb/Host/FileSystem.h:36
   : m_fs(llvm::vfs::getRealFileSystem()), m_collector(nullptr),
-m_home_directory(), m_mapped(false) {}
+m_home_directory() {}
   FileSystem(std::shared_ptr collector)

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103483/new/

https://reviews.llvm.org/D103483

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

2021-06-02 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103483/new/

https://reviews.llvm.org/D103483

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

2021-06-02 Thread Shafik Yaghmour via Phabricator via lldb-commits 
shafik added a comment.

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.




Comment at: lldb/include/lldb/Utility/VMRange.h:29
 
-  VMRange() : m_base_addr(0), m_byte_size(0) {}
+  VMRange()  {}
 

`= default`



Comment at: lldb/source/API/SBBroadcaster.cpp:19
 
-SBBroadcaster::SBBroadcaster() : m_opaque_sp(), m_opaque_ptr(nullptr) {
+SBBroadcaster::SBBroadcaster() : m_opaque_sp() {
   LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBBroadcaster);

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



Comment at: lldb/source/API/SBBroadcaster.cpp:24
 SBBroadcaster::SBBroadcaster(const char *name)
 : m_opaque_sp(new Broadcaster(nullptr, name)), m_opaque_ptr(nullptr) {
   LLDB_RECORD_CONSTRUCTOR(SBBroadcaster, (const char *), name);

We don't need `m_opaque_ptr(nullptr)`



Comment at: lldb/source/API/SBCommandReturnObject.cpp:25
   SBCommandReturnObjectImpl()
-  : m_ptr(new CommandReturnObject(false)), m_owned(true) {}
+  : m_ptr(new CommandReturnObject(false)) {}
   SBCommandReturnObjectImpl(CommandReturnObject &ref)

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



Comment at: lldb/source/API/SBCommandReturnObject.cpp:44
 private:
   CommandReturnObject *m_ptr;
+  bool m_owned = true;

`CommandReturnObject *m_ptr = new CommandReturnObject(false)`


Note, this is ok w/ the other constructors b/c 
[class.base.init/p10](http://eel.is/c++draft/class.base.init#10) 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103483/new/

https://reviews.llvm.org/D103483

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

2021-06-01 Thread Jim Ingham via Phabricator via lldb-commits 
jingham added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103483/new/

https://reviews.llvm.org/D103483

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

2021-06-01 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor added a comment.

+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 :)




Comment at: lldb/include/lldb/Breakpoint/BreakpointOptions.h:47
 CommandData()
-: user_source(), script_source(),
-  interpreter(lldb::eScriptLanguageNone), stop_on_error(true) {}
+: user_source(), script_source() {}
 

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103483/new/

https://reviews.llvm.org/D103483

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

2021-06-01 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103483/new/

https://reviews.llvm.org/D103483

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits