[Lldb-commits] [PATCH] D104488: Create synthetic symbol names on demand to improve memory consumption and startup times.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Some comments on comments...



================
Comment at: lldb/include/lldb/Symbol/Symtab.h:224
+  ///
+  /// We started not always generating symbol names for synthetic symbols and
+  /// we want to be able to lookup synthetic symbols by name when needed. This
----------------
This comment is hard to read.  I think it's mostly because you describe the implementation before the reason for it.

Maybe this would be clearer like:

"We generate unique names for synthetic symbols so that users can look them up by name when needed.  But because doing so is uncommon in normal debugger use, we trade off some performance at lookup time for faster symbol table building by detecting these symbols and generating their names lazily, rather than adding them to the normal symbol indexes.  This function does the job of first consulting the indexes, and if that fails checking whether the symbol has the synthetic symbol prefix and generating the correct synthetic name if it does.




================
Comment at: lldb/source/Symbol/Symbol.cpp:575
+    // identify individual symbols so we give them a unique name. The name
+    // starts with the synthetic symbol prefix and are followed by a unique
+    // number. Typically the UserID is a real symbol is the symbol table
----------------
are -> is

or maybe:

starts with the synthetic symbol prefix, followed by a unique number


================
Comment at: lldb/source/Symbol/Symbol.cpp:576
+    // starts with the synthetic symbol prefix and are followed by a unique
+    // number. Typically the UserID is a real symbol is the symbol table
+    // index of the symbol in the object file's symbol table(s). Synthetic
----------------
is -> of so this reads:

Typically the UserID of a real symbol is ...


================
Comment at: lldb/source/Symbol/Symbol.cpp:578
+    // index of the symbol in the object file's symbol table(s). Synthetic
+    // UserID values tend to be numbers that are higher than the last symbol
+    // index in the symbol table. The synthetic name for the same synthetic
----------------
I don't think you need the implementation detail here, you are stating policy.  Starting from "Typically" I think something like the following is more direct:

Typically the UserID of a real symbol is the symbol table index of the symbol in the object file's symbol table(s), so it will be the same every time you read in the object file.  We want the same persistence for synthetic symbols so that users can identify them across multiple debug sessions, to understand crashes in those symbols and to reliably set breakpoints on them.


================
Comment at: lldb/source/Symbol/Symtab.cpp:628
+  // Synthetic symbol names are not added to the name indexes, but they start
+  // with a prefix and end with a the symbol UserID, so allow users to find
+  // these without having to add them to the name indexes. These queries will
----------------
so -> to


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104488