improve readability and performance of ClangExpressionParser::FindFunctionInModule

[ldrumm]

Summary

• 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 *)

[jingham]

Other than that is looks fine.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

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]

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

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]

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

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

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 :(

[ldrumm]

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

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

lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

ClangExpressionParser::Parse(Stream &stream)
 
     return num_errors;
 }
 
 static bool FindFunctionInModule (ConstString &mangled_name,
                                   llvm::Module *module,
                                   const char *orig_name)
 {
-    for (llvm::Module::iterator fi = module->getFunctionList().begin(), fe = module->getFunctionList().end();
-         fi != fe;
-         ++fi)
+    for (const auto &func : module->getFunctionList())
     {
-        if (fi->getName().str().find(orig_name) != std::string::npos)
+        const StringRef &name = func.getName();
+        if (name.find(orig_name) != StringRef::npos)
         {
-            mangled_name.SetCString(fi->getName().str().c_str());
+            mangled_name.SetString(name);
             return true;
         }
     }
 
     return false;
 }
 
 Error