[PATCH] D148799: [clang] Return `StringRef` from `TargetInfo::getClobbers()`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

You need to clearly state the "why" of the change in the commit message. Sometimes it seems obvious to the author but a reviewer has to assume and may assume incorrectly.

In this case, I guess that since the result of the function only has its length checked then is added to another string, you would avoid a heap allocation for the new std::string temporary by using StringRef.

You could also do this by making a StringRef from the const char* and checking its length. That does leave a small ambiguity as to whether returning nullptr and empty string is the same thing. Which it is, but it's not clear just from the prototype of the function. So changing getClobbers to return StringRef resolves both issues.

But equally, you might have had a completely different goal. Tell me if I am right :)

> Offtop: Is there any discussion platform where I can share some ideas? I have some of them how to refactor TargetInfo classes, but it's a major (and maybe breaking) change I have to discuss prior to implement.

An RFC on discourse is the usual way. If you can supply some work in progress patches or a tree on github then even better (and prevent you from proposing things you later find to be impossible, not that I have ever done that of course :) ).


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

https://reviews.llvm.org/D148799