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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
