[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan marked 8 inline comments as done.
yinghuitan added a comment.

Thanks for all the feedback. I will address the comments and add @clayborg's follow-up suggestions into summary.



================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:47
+lldb::LanguageType SymbolFileOnDemand::ParseLanguage(CompileUnit &comp_unit) {
+  if (!this->m_debug_info_enabled) {
+    Log *log = GetLog();
----------------
DavidSpickett wrote:
> Is there a specific reason to use "this" explicitly instead of just `!m_debug_info_enabled`?
> 
> Something to do with the wrapping of the underlying SymbolFile perhaps.
I will remove from this diff. I guess I just got this habit from working in other languages which may explicitly require `this` otherwise can fail. 



================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:110
+             __FUNCTION__);
+    // Not early exit.
+    return false;
----------------
DavidSpickett wrote:
> What's the meaning of this comment?
Return false from `ForEachExternalModule` tells the caller to not exit early. This is the default value used by `SymbolFile::ForEachExternalModule()`. Let me make it more obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631