Page MenuHomePhabricator

improve readability and performance of ClangExpressionParser::FindFunctionInModule

Authored by ldrumm on Feb 15 2016, 12:09 PM.


  • Prefer llvm::StringRef::find over allocating a std::string and then discarding it after calling the equivalent find method.
  • improve readability of function iterator, and make the function variable a const ref.
  • Prefer passing StringRef directly to ConstString::setString(StringRef &) over allocating a new std::string from a StringRef only to convert to const char *, which is then passed to ConstString::setCString(const char *)

Diff Detail


Event Timeline

ldrumm updated this revision to Diff 48003.Feb 15 2016, 12:09 PM
ldrumm retitled this revision from to improve readability and performance of ClangExpressionParser::FindFunctionInModule.
ldrumm updated this object.
ldrumm added reviewers: spyffe, dawn.
ldrumm added a subscriber: lldb-commits.

Other than that is looks fine.

559–560 ↗(On Diff #48003)

Don't make this kind of change, please. As long as the arguments fit in 120 characters we don't have a rule one way or the other about how to write argument lists like this. But changing them just because they look better to you results in unnecessary churn. Moreover, this is changing it away from the way all the other functions in this source file are written, so it ends up looking odd.

ldrumm added inline comments.Feb 16 2016, 4:34 AM
559–560 ↗(On Diff #48003)

This change is made by clang-fomat using the rules in the lldb .clang-format file.

I'm willing to revert this part of the commit, but seeing as this change is essentially a refactoring of the whole method, it feels natural to also format the prototype while I’m at it.

dawn accepted this revision.Feb 24 2016, 1:53 PM
dawn edited edge metadata.

lgtm other than nitpick on formatting. And sorry for delayed review - been sick.

559–560 ↗(On Diff #48003)

Please just keep the space before the params and the return type on a separate line. Sadly, we can't use clang-format for function decls/defs in lldb because it doesn't support the lldb-style here, so the formatting of this must be done manually :(

This revision is now accepted and ready to land.Feb 24 2016, 1:53 PM
This revision was automatically updated to reflect the committed changes.
ldrumm added inline comments.Feb 25 2016, 5:14 AM
559–560 ↗(On Diff #48003)

Thanks for your comments, Dawn. I appreciate the time it takes - especially after being ill :(

@jingham @dawn
To step around the style questions completely, I've updated the changelist to ignore the prototype entirely. I hope that suits you both.

Thanks again