Page MenuHomePhabricator

[lldb] NFC: refactor CompileUnit::ResolveSymbolContext
ClosedPublic

Authored by kwk on Nov 27 2019, 1:52 AM.

Details

Summary

I found the above named method hard to read because it had

a) many nested blocks,
b) one return statement at the end with some logic involved,
c) a duplicated while-loop with just small differences in it.

I decided to refactor this function by employing an early exit strategy.
In order to capture the logic in the return statement and to not have it
repeated more than once I chose to implement a very small lamda function
that captures all the variables it needs.
I also replaced the two while-loops with just one.

This is a non-functional change (NFC).

Diff Detail

Event Timeline

kwk created this revision.Nov 27 2019, 1:52 AM
Herald added a project: Restricted Project. · View Herald Transcript
kwk planned changes to this revision.Nov 27 2019, 1:54 AM
kwk updated this revision to Diff 231201.Nov 27 2019, 1:59 AM

Small changes

teemperor requested changes to this revision.Nov 27 2019, 2:01 AM
teemperor added a subscriber: teemperor.

Let's just remove that return value. It's anyway only used in one place or so where we can just call GetSize() manually. Then we also don't need the lambda. Otherwise this LGTM, thanks!

This revision now requires changes to proceed.Nov 27 2019, 2:01 AM
kwk edited the summary of this revision. (Show Details)Nov 27 2019, 2:03 AM
labath added a subscriber: labath.Nov 27 2019, 2:03 AM

What we've started doing these days is to just remove the "number of things appended" return value from these kinds of functions, as they make it very easy to introduce bugs. (And most callers don't care about those, and those that do, can implement this easily on their own.)

It doesn't look like this function has that many callers, so it should be easy to do the same here...

kwk added a comment.Nov 27 2019, 2:04 AM

Let's just remove that return value. It's anyway only used in one place or so where we can just call GetSize() manually. Then we also don't need the lambda. Otherwise this LGTM, thanks!

Okay, fair enough. Let me do the changes.

kwk planned changes to this revision.Nov 27 2019, 2:13 AM
kwk updated this revision to Diff 231406.Nov 28 2019, 4:22 AM
  • Have ResolveSymbolContext not return anything
This revision is now accepted and ready to land.Nov 28 2019, 4:45 AM
This revision was automatically updated to reflect the committed changes.
kwk reopened this revision.Nov 28 2019, 7:52 AM

Reopening because I did find the logic error.

This revision is now accepted and ready to land.Nov 28 2019, 7:52 AM
kwk updated this revision to Diff 231440.Nov 28 2019, 7:55 AM
  • Fixed logic error
kwk planned changes to this revision.Nov 28 2019, 7:56 AM

Running tests locally.

kwk requested review of this revision.Nov 28 2019, 8:03 AM
This revision is now accepted and ready to land.Nov 28 2019, 8:03 AM
labath accepted this revision.Nov 28 2019, 8:20 AM
This revision was automatically updated to reflect the committed changes.