Page MenuHomePhabricator

Migrate to llvm::unique_function instead of static member functions for callbacks
ClosedPublic

Authored by nealsid on Aug 4 2018, 3:31 PM.

Details

Summary

A few cleanups suggested in another patch review's comments:

  1. Use llvm:unique_function for storing & invoking callbacks from Editline to IOHandler
  2. Change return type of one of the callback setters from bool to void, since it's return value was never used
  3. Moved the callback setters inline & made them nonstatic, since that's more consistent with other setter definitions
  4. Removed the baton parameter since we no longer need it anymore

Diff Detail

Repository
rLLDB LLDB

Event Timeline

nealsid created this revision.Aug 4 2018, 3:31 PM
nealsid updated this revision to Diff 159195.Aug 4 2018, 3:39 PM

Fix unit test build

zturner added inline comments.Aug 4 2018, 4:01 PM
include/lldb/Host/Editline.h
94

Try llvm::unique_function instead of std::function (in other locations as well). I should have mentioned this earlier but forgot. It's basically the same except it doesn't require the callable to be copy constructible, which enables some optimizations. Not that this is a hot-path, but if we can use something strictly better, might as well.

179

= std::move(callback)

source/Core/IOHandler.cpp
314–315

I think we should avoid std::bind. Instead, just write

m_editline_ap->SetIsInputCompleteCallback([this](EditLine* a, StringList& b) { return IsInputCompleteCallback(a, b); });

similary for other places. I'd argue that std::bind actually makes the code *worse* since it's so hard to construct these bind statements and figure out what they do.

Eugene.Zelenko added inline comments.
include/lldb/Host/Editline.h
92

Please use using. Same in other places.

source/Core/IOHandler.cpp
315

See also Clang-tidy modernize-avoid-bind.

nealsid planned changes to this revision.Aug 4 2018, 10:02 PM

Thanks for the comments! Will address and resend for review.

nealsid added inline comments.Aug 5 2018, 3:31 PM
include/lldb/Host/Editline.h
94

Ok, cool - so if I understand the optimizations we're trying to make, we want to make sure to use move semantics if were can. Doesn't std::function use move semantics when constructing itself from rvalue references to lambdas? Even if it didn't, would we benefit from llvm::unique_function since our cases are pretty simple?

zturner added a subscriber: nealsid.Aug 5 2018, 4:26 PM

std::function might be able to construct itself efficiently from a lambda,
but once it’s in the std::function all that info is lost and moving it
around more will always copy (unless you explicitly move). We only really
assign it once, so this isn’t an issue, but unique_function is nice from a
purist point of view in that it won’t ever allow std::function’s slow path
to be taken, even accidentally.

In practice this is a non issue because we’re talking about a couple of
cycles in code that isn’t even in a hot path, but it’s still nice for the
same reason you might use a singly linked list instead of a doubly linked
for a queue. It gives you as much as you need and nothing more.

So the benefit isn’t really one of performance. It’s more just a mindset
of, rather than saying “I’ll use the fast one if performance is a concern”,
saying instead “I’ll only use the slow one if it’s necessary for my use
case”

nealsid updated this revision to Diff 159317.Aug 6 2018, 9:23 AM
nealsid marked 5 inline comments as done.

Addressed code review feedback to move to llvm::unique_function instead of function pointers for callbacks and type aliases instead of typedefs.

nealsid marked an inline comment as done.Aug 6 2018, 9:24 AM
nealsid added inline comments.
source/Core/IOHandler.cpp
315

I also ran clang-tidy and there's a few unrelated suggested cleanups to make, but I don't know what options to use to be consistent with the rest of the project.

nealsid updated this revision to Diff 159319.Aug 6 2018, 9:27 AM
nealsid set the repository for this revision to rLLDB LLDB.
nealsid updated this revision to Diff 159394.Aug 6 2018, 2:25 PM
zturner accepted this revision.Aug 13 2018, 2:39 PM

Sorry for the delay, was on vacation last week.

This revision is now accepted and ready to land.Aug 13 2018, 2:39 PM
nealsid edited the summary of this revision. (Show Details)Aug 31 2018, 8:59 AM
nealsid edited the summary of this revision. (Show Details)Aug 31 2018, 9:00 AM
nealsid updated this revision to Diff 163548.Aug 31 2018, 9:11 AM

Fix 80 char line length violations

nealsid retitled this revision from Migrate to std::function instead of static member functions for callbacks to Migrate to llvm::unique_function instead of static member functions for callbacks.Aug 31 2018, 9:11 AM

Fix 80 char line length violations

Just curious, how did you fix them? Was it by running clang-format, or manually?

nealsid updated this revision to Diff 163559.Aug 31 2018, 10:29 AM

Ran clang-format

zturner accepted this revision.Aug 31 2018, 2:01 PM
nealsid updated this revision to Diff 326553.Feb 25 2021, 4:50 PM

I'm updating this very old patch that I forgot about in 2018. This one removes the use of batons in the edit line callbacks, replaces typedefs with using declarations, and uses unique_function instead of callbacks.
Thanks,
Neal

Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 4:50 PM

Thanks Neal. Can you run clang-format again and upload the diff with context (git diff -U9999). Overall this looks pretty good.

Thanks Neal. Can you run clang-format again and upload the diff with context (git diff -U9999). Overall this looks pretty good.

Thanks for the review. I have run clang-format and am currently running through a bunch of testing with the completion/suggestion features just to make sure. Hope to get it updated in the next couple of days.

nealsid updated this revision to Diff 326954.Feb 27 2021, 11:33 PM
nealsid removed a project: Restricted Project.

Updated diff with run of clang-format and added a check for function pointer validity. Thanks.

nealsid added a project: Restricted Project.Feb 27 2021, 11:33 PM
JDevlieghere accepted this revision.Mar 1 2021, 11:48 AM

LGTM!

lldb/include/lldb/Host/Editline.h
60 ↗(On Diff #326954)
nealsid updated this revision to Diff 327317.Mar 1 2021, 4:22 PM

Removed comment on #include line and organized the #includes to match coding guidelines better (I wasn't sure about whether the project uses blank lines in between #includes from different subproject like lldb/clang/llvm so I didn't make the change more broadly)

nealsid marked an inline comment as done.Mar 1 2021, 4:36 PM

Removed comment on #include line and organized the #includes to match coding guidelines better (I wasn't sure about whether the project uses blank lines in between #includes from different subproject like lldb/clang/llvm so I didn't make the change more broadly)

Looks good. Let me know if you don't have commit access and need me to land this for you.

Removed comment on #include line and organized the #includes to match coding guidelines better (I wasn't sure about whether the project uses blank lines in between #includes from different subproject like lldb/clang/llvm so I didn't make the change more broadly)

Looks good. Let me know if you don't have commit access and need me to land this for you.

I do not have commit access, so if you could land it, that would be great. Appreciate the review.

This revision was landed with ongoing or failed builds.Mar 2 2021, 4:14 PM
This revision was automatically updated to reflect the committed changes.