[PATCH] D139720: [clang][PPC] Checking Unknown Values Passed to -mcpu

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 marked 2 inline comments as done.
qiongsiwu1 added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/PPC.cpp:77
+                             .Default(CPUName);
+    return TargetCPUName.str();
   }
----------------
jamieschmeiser wrote:
> qiongsiwu1 wrote:
> > jamieschmeiser wrote:
> > > Why did you change the type from const char *?  Couldn't you use CPUName->data() in the default instead?  With your change, I think it may need to create StringRefs around all of the choices and then just get the string from them.
> > I was worried about the [[ https://github.com/llvm/llvm-project/blob/d3c3de63ce8416ab2dee7f784e54b00a2aa8ed85/llvm/include/llvm/ADT/StringRef.h#L129 | comment ]] on top of `StringRef::data()` which says "Get a pointer to the start of the string (which may not be null terminated).". I was not sure what would happen if the data inside `CPUName` was not null terminated (it might be fine) so I thought it would be safer to use the `StringRef`s directly instead. 
> Probably the same sort of thing that would happen with creating a StringRef since that calls strlen to set the size of the string it is wrapping :-)
Got it! Thanks for the suggestion/clarification! The patch is updated to use `const char *`. I looked around and I do see `StringRef::data()` used quite liberally here and there (e.g. [[ https://github.com/llvm/llvm-project/blob/9466b49171dc4b21f56a48594fc82b1e52f5358a/clang/lib/Driver/ToolChains/Gnu.cpp#L867 | here ]]). So I agree in this case we should be fine. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139720