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

[David Blaikie via Phabricator via lldb-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",
----------------
labath wrote:
> dblaikie wrote:
> > 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?
> > Is there no API migration strategy in the SB API? Introducing new versions of functions and deprecating old ones?
> 
> Right now, there isn't. However, I am not very happy with SB layer restrictions constraining the internal APIs. One of the ways around that (which is already used in a some places) is to pass the value through a ConstString at the SB layer. This will guarantee that the string is null terminated and persistent (at the cost of some cpu and memory). Though I am not very fond of that solution either, it does not seem like too bad of an option for this case, as the set of language names is limited.
> 
> As for testing of logging, there definitely are ways to test that, but we (usually) don't do that. I think the best analogy here would be the LLVM_DEBUG output in llvm. This is also mainly a debugging aid, and we usually don't have dedicated tests for that (though it would certainly be possible to do that). I think both mechanisms would benefit from some testing, but the question is how much testing, and whether that time wouldn't be better spent improving test coverage elsewhere...
Fair enough. Pity, but ah well.

Might be possible to add a unit test that demonstrates the SFINAE at work (I don't know for sure, though - you'd need to introduce an overload set that would've preferred toStringRef(bool) before the change, but now prefers some overload in the unit test with different behavior you could use to differentiate the two & show the SFINAE at work)


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

https://reviews.llvm.org/D65942