[Lldb-commits] [PATCH] D99989: [lldb-vscode] Distinguish shadowed variables in the scopes request

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2934
+    // We first find out which variable names are duplicated
+    std::unordered_map<const char *, int> variable_name_counts;
+    for (auto i = start_idx; i < end_idx; ++i) {
----------------
clayborg wrote:
> We are getting a variable name via the public API which returns a "const char *". The only way this works is if the string comes from a ConstString otherwise there are lifetime issues. So this is and should be ok. The diff you are talking about was from internal LLDB code. For internal LLDB code we can enforce this by ensuring that people use ConstString, but externally through the API, we assume any "const char *" that comes out of it is uniqued and comes form a ConstString.
I don't think there is any promise that every `const char *` coming from the SB API is generated from a ConstString. If there was such a promise a good chunk of the SB API would already violate it.

That the `const char *` needs to have a reasonable life time (e.g., the same life time as the SB API object that returns it) seems like a basic foundation to have the API work at all. But promising that they are uniquified really serves essentially no purpose? I sure hope that simply returning string literals doesn't suddenly become an API violation (or that we need to forever store every generated error message that we return from the SB API).

Also I think my example is spot on: An unverified assumption that doesn't hold true. And once we change the underlying code someone has to spend a week tracking down bogus string comparisons in the year 2021.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99989