[Lldb-commits] [PATCH] D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance

[Frederic Riss via Phabricator via lldb-commits]

friss added inline comments.


================
Comment at: source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp:274
+      vtable_first_entry_resolved.CalculateSymbolContextCompileUnit();
+  llvm::StringRef name_to_use = func_to_match;
+  bool is_lambda = contains_lambda_identifier(name_to_use);
----------------
Why a new variable?


================
Comment at: source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp:287
+  size_t pos = llvm::StringRef::npos;
+  pos = name_to_use.find("::operator");
+
----------------
Doesn't matter too much for lambdas, but shouldn't this be "operator()" ?
Is this guaranteed to return a valid index? Should we assert on it?


================
Comment at: source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp:293
+      if (name.startswith(sliced_name) && name.contains("operator"))
+        if (name.contains("$_") || name.contains("'lambda'"))
+          return true;
----------------
this is basically is_lambda. As we filter it above, why do we need this test?


================
Comment at: source/Symbol/CompileUnit.cpp:101
+  // m_functions_by_uid is filled in lazily but we need all the entries.
+  symbol_file->ParseFunctions(*this);
+
----------------
This seems pretty expensive. Can we Force the parsing of the functions just in the compiler unit rather than the whole symbol file? If you are using a dSYM, then this is going to load all the debug information in one go.


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

https://reviews.llvm.org/D69913