[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:966
             name = "(internal)";
           result.GetOutputStream().Printf(
+              "%d: %s, module %s, function %s{%s}%s\n", recognizer_id,
----------------
We should stream to the output stream directly to avoid all these useless conversions. 


================
Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:29
 ///    Otherwise, returns \a llvm::None.
-llvm::Optional<std::tuple<FileSpec, StringRef>>
+llvm::Optional<std::tuple<FileSpec, StringRef, StringRef>>
 GetAbortLocation(Process *process) {
----------------
With two elements in the tuple I was torn between a struct with named fields, but with 3 fields I strongly believe that would be the better choice. 


================
Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:152
+    ConstString func_name = sym_ctx.GetFunctionName();
+    if (func_name == ConstString(function_name) ||
+        alternate_function_name.empty() ||
----------------
I believe someone added an overload for comparing ConstStrings with StringRefs. We shouldn't have to pay the price of creating one here just for comparison. Same below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252