[PATCH] D65942: Disallow implicit conversion from pointers to bool in llvm::toStringRef

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lldb/source/Symbol/TypeSystem.cpp:331
           "TypeSystem for language " +
-              llvm::toStringRef(Language::GetNameForLanguageType(language)) +
+              llvm::StringRef(Language::GetNameForLanguageType(language)) +
               " doesn't exist",
----------------
teemperor wrote:
> dblaikie wrote:
> > Perhaps Language::GetNameForLanguageType should return StringRef to start with?
> > 
> > (& guessing there should be an lldb test case demonstrating this fix?)
> The problem is that this is used in LLDB's SB API which returns a const char* (and we can't change that). So we can't return a StringRef as that doesn't convert back to const char * (and we can't go over std::string or anything like that as the API doesn't transfer ownership back to the caller). I added an alternative method to save some typing (and maybe we can go fully StringRef in the future if we ever change the SB API strings).
> 
> And we don't test log message content in LLDB. Also the log message only happens on systems with unsupported type system (like MIPS or something like that). And we don't have any such bot that would even run the test.
Those both seem a bit problematic... 

Is there no API migration strategy in the SB API? Introducing new versions of functions and deprecating old ones?

And testing - clearly this code was buggy and worth fixing, so it ought to be worth testing. Any chance of unit testing/API-level testing any of this?


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

https://reviews.llvm.org/D65942