This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Simplify Boolean expressions
ClosedPublic

Authored by JDevlieghere on Dec 11 2018, 2:41 PM.

Details

Summary

After creating D55574, clang-tidy was of course fresh in my mind and I noticed another common pattern that can be improved.

https://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html

Command used:

run-clang-tidy.py -checks='-*,readability-simplify-boolean-expr' -format -fix $PWD

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 11 2018, 2:41 PM

This one is probably less controversial than D55574 :-)

include/lldb/Target/ThreadPlanTracer.h
46

is this actually better? Perhaps.

source/DataFormatters/TypeCategoryMap.cpp
122

this probably should be a while loop, or the initializer should go in the for.

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
363

here the indentation actually got worse

I find the static_cast<bool> to be a bit too clever, I don't think it helps readability.

This is also too large to digest in a way I would feel satisfied that I did not miss anything.

Eugene.Zelenko retitled this revision from Simplify boolean expressions to [LLDB] Simplify Boolean expressions.Dec 11 2018, 5:13 PM

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.

JDevlieghere marked 3 inline comments as done.

Feedback Adrian

labath added a subscriber: labath.Dec 12 2018, 1:34 AM

I also find the static_cast<bool> 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.

I also find the static_cast<bool> 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 :-)

aprantl accepted this revision.Dec 12 2018, 9:25 AM
This revision is now accepted and ready to land.Dec 12 2018, 9:25 AM

Remove changes that introduced static_cast<bool>

shafik accepted this revision as: shafik.Dec 12 2018, 10:02 AM

LGTM after comment are addressed.

source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
5894

This looks like a big chunk of unrelated changes.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1725–1728

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

source/Plugins/Process/mach-core/ProcessMachCore.cpp
305–320

Another big chunk of unrelated changes.

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
1117

The static_cast feels less readable here.

source/Symbol/LineTable.cpp
217

static_cast

236

static_cast

source/Symbol/Type.cpp
749

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

source/Utility/RegisterValue.cpp
478

Nice one.

This revision was automatically updated to reflect the committed changes.