[Lldb-commits] [PATCH] D55584: [LLDB] Simplify Boolean expressions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik accepted this revision as: shafik.
shafik added a comment.

LGTM after comment are addressed.




Comment at: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5894
+if (!success) {
+  offset = MachHeaderSizeFromMagic(m_header.magic);
+  for (uint32_t i = 0; !success && i < m_header.ncmds; ++i) {

This looks like a big chunk of unrelated changes.



Comment at: 
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:1725
 // not support qHostInfo or qWatchpointSupportInfo packets.
-if (atype == llvm::Triple::mips || atype == llvm::Triple::mipsel ||
-atype == llvm::Triple::mips64 || atype == llvm::Triple::mips64el ||
-atype == llvm::Triple::ppc64le)
-  after = false;
-else
-  after = true;
+after =
+!(atype == llvm::Triple::mips || atype == llvm::Triple::mipsel ||

Not sure if I like this one, the previous form feels more readable.



Comment at: source/Plugins/Process/mach-core/ProcessMachCore.cpp:305
   std::string corefile_identifier = core_objfile->GetIdentifierString();
-  if (found_main_binary_definitively == false 
-  && corefile_identifier.find("Darwin Kernel") != std::string::npos) {
-  UUID uuid;
-  addr_t addr = LLDB_INVALID_ADDRESS;
-  if (corefile_identifier.find("UUID=") != std::string::npos) {
-  size_t p = corefile_identifier.find("UUID=") + strlen("UUID=");
-  std::string uuid_str = corefile_identifier.substr(p, 36);
-  uuid.SetFromStringRef(uuid_str);
-  }
-  if (corefile_identifier.find("stext=") != std::string::npos) {
-  size_t p = corefile_identifier.find("stext=") + strlen("stext=");
-  if (corefile_identifier[p] == '0' && corefile_identifier[p + 1] == 
'x') {
-  errno = 0;
-  addr = ::strtoul(corefile_identifier.c_str() + p, NULL, 16);
-  if (errno != 0 || addr == 0)
-  addr = LLDB_INVALID_ADDRESS;
-  }
-  }
-  if (uuid.IsValid() && addr != LLDB_INVALID_ADDRESS) {
-  m_mach_kernel_addr = addr;
-  found_main_binary_definitively = true;
-  if (log)
-log->Printf("ProcessMachCore::DoLoadCore: Using the kernel address 
0x%" PRIx64
-" from LC_IDENT/LC_NOTE 'kern ver str' string: '%s'", 
addr, corefile_identifier.c_str());
+  if (!found_main_binary_definitively &&
+  corefile_identifier.find("Darwin Kernel") != std::string::npos) {

Another big chunk of unrelated changes.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp:1117
   if ((distance(range.first, range.second) == 1) &&
-  range.first->end_sequence == true) {
+  static_cast(range.first->end_sequence)) {
 *range.first = state;

The `static_cast` feels less readable here.



Comment at: source/Symbol/LineTable.cpp:217
 if (prev_pos->file_addr == search_entry.file_addr &&
-prev_pos->is_terminal_entry == false)
+!static_cast(prev_pos->is_terminal_entry))
   --pos;

static_cast



Comment at: source/Symbol/LineTable.cpp:236
 // terminating entry for a previous line...
-if (pos != end_pos && pos->is_terminal_entry == false) {
+if (pos != end_pos && !static_cast(pos->is_terminal_entry)) {
   uint32_t match_idx = std::distance(begin_pos, pos);

static_cast



Comment at: source/Symbol/Type.cpp:749
 bool TypeAndOrName::IsEmpty() const {
-  if ((bool)m_type_name || (bool)m_type_pair)
-return false;
-  else
-return true;
+  return !((bool)m_type_name || (bool)m_type_pair);
 }

Oh, even worse C-style casts ... can we remove these. I am assuming they are 
triggering a member conversion function.



Comment at: source/Utility/RegisterValue.cpp:478
   if (this == )
-return rhs.m_type == eTypeInvalid ? false : true;
+return rhs.m_type != eTypeInvalid;
 

Nice one.


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

https://reviews.llvm.org/D55584



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

[Lldb-commits] [PATCH] D55584: [LLDB] Simplify Boolean expressions

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D55584#1328087 , @labath wrote:

> I also find the `static_cast` thingy weird. The rest of the changes 
> seem to be towards the better (based on a pseudo-random sample), but the 
> change is a quite big.


Me and Adrian went through all the changes and I didn't see anything that 
looked out of the ordinary.

I left the static cast because it was suggested to me before in a review, but 
happy to remove it as there is a consensus that it's worse :-)


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

https://reviews.llvm.org/D55584



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

[Lldb-commits] [PATCH] D55584: [LLDB] Simplify Boolean expressions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I also find the `static_cast` thingy weird. The rest of the changes seem 
to be towards the better (based on a pseudo-random sample), but the change is a 
quite big.


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

https://reviews.llvm.org/D55584



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

[Lldb-commits] [PATCH] D55584: [LLDB] Simplify Boolean expressions

[Eugene Zelenko via Phabricator via lldb-commits]

Eugene.Zelenko added a comment.

Thank you for cleanup effort!

I would suggest to also run modernize checks and at least next readability 
checks:

readability-container-size-empty
readability-isolate-declaration
readability-redundant-control-flow
readability-redundant-member-init
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init

Indeed, Clang-tidy offers even more useful checks.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55584



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