Page MenuHomePhabricator

1/2: [nfc] [lldb] Unindent code
ClosedPublic

Authored by jankratochvil on Apr 2 2020, 11:49 AM.

Details

Summary

It removes some needless deep indentation and some redundant statements.
It prepares the code for a more clean next patch - DWARF index callbacks D77327.

Diff Detail

Event Timeline

jankratochvil created this revision.Apr 2 2020, 11:49 AM
jankratochvil retitled this revision from [nfc] [lldb] Unindent code to 1/2: [nfc] [lldb] Unindent code.Apr 2 2020, 11:56 AM
kwk added a comment.Apr 2 2020, 1:01 PM

Overall looks good to me except for one larger logic change. Maybe a your comment can clarify this. The code was in a very bad shape before, given the countless amounts of shortcuts you could take.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2019

I assume this can be removed because you're iterating over num_matches == 0 when it's empty. But I cannot tell about the DWARFDebugInfo &debug_info = dwarf->DebugInfo(); and how costly this call is and if it makes sense to run when the offsets are empty.

lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
525

Why is the if no longer needed?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2067

Nice shortcut.

2957

This looks like a logic change and I wonder if it should go in a separate patch, maybe?

I am generally very okay with this change, though some changes look suspicious, as @kwk noted.

jankratochvil marked 6 inline comments as done.Apr 2 2020, 1:55 PM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2019

I did not put an attention to it, thanks for catching it. Personally I think it is OK but when it has been brought up I am fixing this possible performance regression. SymbolFileDWARF::DebugInfo() contains some llvm::call_once(m_info_once_flag which does have some cost.

lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
525

It will now run DWARFMappedHash::ExtractDIEArray on empty array which is cheap, it does nothing.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2957

This should be NFC, do I miss something?
From:

if (a == b) {
  c;
}

The patch changes it to:

if (a != b)
  continue;
c;
jankratochvil marked 2 inline comments as done.
shafik added a comment.Apr 2 2020, 2:14 PM

I feel like this should have been split in two. It feels like there are straight forward obviously correct changes and a several others that require some thinking about but look correct.

lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
525

If we stick with this change, please add a comment explaining this. It is not obvious and someone later on may come and change it back thinking this is a bug.

I feel like this should have been split in two. It feels like there are straight forward obviously correct changes and a several others that require some thinking about but look correct.

That's a really good point. I agree. Can you split all changes of one kind into separate patches?

kwk accepted this revision.Apr 3 2020, 12:52 AM

No need to split this into multiples revisions. It is just a rewrite of some odd nesting.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2957

Ah, now I see that it was previously a block on the right side of the diff :).

This revision is now accepted and ready to land.Apr 3 2020, 12:52 AM
jankratochvil marked 2 inline comments as done.Apr 3 2020, 1:44 AM
jankratochvil marked 2 inline comments as done.Apr 3 2020, 11:55 AM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
525

I have rather dropped this part of this refactorization patch. It will look more logical together with changes of the next part D77327. Thanks for the comments.

jankratochvil marked an inline comment as done.

I have checked in the really obvious parts as rG80237523193d.
The remainder is left here - although I do not see there more problematic parts now.

jankratochvil edited the summary of this revision. (Show Details)Apr 3 2020, 1:10 PM

Does anyone know why harbormaster is useless?

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2019

not costly at all. This will be fine.

lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
69–72

This logic is easy to understand by reading the code.

84–85

This logic is harder to follow. Maybe:

if (!(die_tag == 0 || die_tag == DW_TAG_class_type || die_tag == DW_TAG_structure_type))
  continue;
jankratochvil marked 4 inline comments as done.Apr 4 2020, 1:03 AM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
84–85

OK, changed it.
I will follow this also IMO more readable form, I changed it since a 10 years old review but that does not matter anymore.

jankratochvil marked an inline comment as done.
labath added a comment.Apr 6 2020, 2:09 AM

Looks good to me, but let's wait for @shafik and @aprantl to take a look.

aprantl accepted this revision.Apr 6 2020, 1:47 PM
aprantl added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2021

Can this be made into a range-based for loop in a separate commit?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2063–2069

same here (later)

2929

These two continues IMO are a bit confusing to read this way. Perhaps in this case an if (log) block with just one continue at the end is easier to read. Up to you.

2965

I think I'd prefer a return over a break, (iff they are equivalent!).

shafik added inline comments.Apr 6 2020, 7:31 PM
lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
70–71

I also feel like the previous version was more readable. Is there another gain that I am missing?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2071–2072

This whole switch feels overly clever, how about:

if (die.Tag() != DW_TAG_variable)
  continue;
jankratochvil marked 10 inline comments as done.Apr 7 2020, 2:08 PM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2021

In D77327 it gets changed into a callback anyway so method_die_offsets no longer exists there.

lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
70–71

I find both versions OK but this patch is going to revert my change in rG80237523193d so everyone should be happy.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2063–2069

In D77327 it gets changed into a callback anyway so die_offsets no longer exists there.

2929

OK, changed back to if (log) { ... }. I even was not sure with that.

jankratochvil marked 4 inline comments as done.

Implemented all the review comments.

jankratochvil marked 2 inline comments as done.Apr 7 2020, 2:20 PM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2949

This break cannot be changed to return as in D77327 it will become a callback lambda.

2965

Wrote above - This break cannot be changed to return as in D77327 it will become a callback lambda.

aprantl accepted this revision.Apr 9 2020, 10:58 AM
This revision was automatically updated to reflect the committed changes.