[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D147833#4253416 <https://reviews.llvm.org/D147833#4253416>, @jasonmolenda wrote:

> idk maybe I'm over-thinking it, but this does make me a little uncomfortable.  We have at least one instance where someone did `platform_sp->GetPluginName() == "ios-simulator"` in ClangExpressionSourceCode.cpp (probably to avoid a dependency on an an apple platform plugin in generic code; the code should undoubtedly be done differently) and the mach-o linker on darwin today will unique identical c-strings from separate compilation units, but I doubt that's a guarantee on Darwin or on other platforms.  Platform::GetPluginName is returning a StringRef here, and we naturally see code like this and assume the c-string will be converted to a StringRef or ConstString implicitly for the comparison operator.  But if GetPluginName started returning a pointer to const c-string and the strings aren't uniqued, the comparison fails in a tricky to see way.



In D147833#4253422 <https://reviews.llvm.org/D147833#4253422>, @jasonmolenda wrote:

> In D147833#4253416 <https://reviews.llvm.org/D147833#4253416>, @jasonmolenda wrote:
>
>> idk maybe I'm over-thinking it, but this does make me a little uncomfortable.  We have at least one instance where someone did `platform_sp->GetPluginName() == "ios-simulator"` in ClangExpressionSourceCode.cpp
>
> I only looked around for the first instance of this code pattern, there may be another place where we compare identification strings from objects.  And I don't feel confident that I can anticipate what will be added to lldb in the future.

Yes, we definitely do that in many places. I see your point though, comparing const c-string pointers can get pretty tricky if somebody refactors this in the future. In that case, instead of `GetFlavor` returning a `const char *` or a `ConstString`, it could return a `StringRef` and this should be handled, no? Just like `GetPluginName()` in your example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833