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