[llvm] r283018 - Use StringRef instead of raw pointers in MCAsmInfo/MCInstrInfo APIs (NFC)
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Sat Oct 1 09:15:08 PDT 2016
> On Oct 1, 2016, at 9:08 AM, Zachary Turner <zturner at google.com> wrote:
>
>
>
> On Sat, Oct 1, 2016 at 8:52 AM Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
>> On Oct 1, 2016, at 8:08 AM, Zachary Turner <zturner at google.com <mailto:zturner at google.com>> wrote:
>>
>
>> On Fri, Sep 30, 2016 at 11:55 PM Mehdi Amini via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> Author: mehdi_amini
>> Date: Sat Oct 1 01:46:33 2016
>> New Revision: 283018
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=283018&view=rev <http://llvm.org/viewvc/llvm-project?rev=283018&view=rev>
>> Log:
>> Use StringRef instead of raw pointers in MCAsmInfo/MCInstrInfo APIs (NFC)
>>
>> Modified:
>> llvm/trunk/include/llvm/MC/MCAsmInfo.h
>> llvm/trunk/include/llvm/MC/MCInstrInfo.h
>> llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp
>> llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp
>> llvm/trunk/lib/MC/MCDisassembler/Disassembler.cpp
>> llvm/trunk/lib/MC/MCParser/AsmLexer.cpp
>> llvm/trunk/lib/Target/Hexagon/HexagonInstrInfo.cpp
>> llvm/trunk/lib/Target/Hexagon/MCTargetDesc/HexagonMCInstrInfo.cpp
>> llvm/trunk/lib/Target/Hexagon/MCTargetDesc/HexagonMCInstrInfo.h
>> llvm/trunk/lib/Target/Mips/Mips16InstrInfo.cpp
>> llvm/trunk/lib/Target/X86/Disassembler/X86Disassembler.cpp
>> llvm/trunk/lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
>> llvm/trunk/lib/Target/X86/Disassembler/X86DisassemblerDecoder.h
>> llvm/trunk/tools/llvm-objdump/MachODump.cpp
>>
>> Modified: llvm/trunk/include/llvm/MC/MCAsmInfo.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCAsmInfo.h?rev=283018&r1=283017&r2=283018&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCAsmInfo.h?rev=283018&r1=283017&r2=283018&view=diff>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/MC/MCAsmInfo.h (original)
>> +++ llvm/trunk/include/llvm/MC/MCAsmInfo.h Sat Oct 1 01:46:33 2016
>> @@ -103,7 +103,7 @@ protected:
>>
>> /// This indicates the comment character used by the assembler. Defaults to
>> /// "#"
>> - const char *CommentString;
>> + StringRef CommentString;
>>
>> /// This is appended to emitted labels. Defaults to ":"
>> const char *LabelSuffix;
>> @@ -117,17 +117,17 @@ protected:
>> /// This prefix is used for globals like constant pool entries that are
>> /// completely private to the .s file and should not have names in the .o
>> /// file. Defaults to "L"
>> - const char *PrivateGlobalPrefix;
>> + StringRef PrivateGlobalPrefix;
>>
>> /// This prefix is used for labels for basic blocks. Defaults to the same as
>> /// PrivateGlobalPrefix.
>> - const char *PrivateLabelPrefix;
>> + StringRef PrivateLabelPrefix;
>>
>> /// This prefix is used for symbols that should be passed through the
>> /// assembler but be removed by the linker. This is 'l' on Darwin, currently
>> /// used for some ObjC metadata. The default of "" meast that for this system
>> /// a plain private symbol should be used. Defaults to "".
>> - const char *LinkerPrivateGlobalPrefix;
>> + StringRef LinkerPrivateGlobalPrefix;
>>
>> /// If these are nonempty, they contain a directive to emit before and after
>> /// an inline assembly statement. Defaults to "#APP\n", "#NO_APP\n"
>> @@ -468,17 +468,17 @@ public:
>> /// printed.
>> unsigned getCommentColumn() const { return 40; }
>>
>> - const char *getCommentString() const { return CommentString; }
>> + StringRef getCommentString() const { return CommentString; }
>> const char *getLabelSuffix() const { return LabelSuffix; }
>>
>> bool useAssignmentForEHBegin() const { return UseAssignmentForEHBegin; }
>> bool needsLocalForSize() const { return NeedsLocalForSize; }
>> - const char *getPrivateGlobalPrefix() const { return PrivateGlobalPrefix; }
>> - const char *getPrivateLabelPrefix() const { return PrivateLabelPrefix; }
>> + StringRef getPrivateGlobalPrefix() const { return PrivateGlobalPrefix; }
>> + StringRef getPrivateLabelPrefix() const { return PrivateLabelPrefix; }
>> bool hasLinkerPrivateGlobalPrefix() const {
>> return LinkerPrivateGlobalPrefix[0] != '\0';
>> }
>> - const char *getLinkerPrivateGlobalPrefix() const {
>> + StringRef getLinkerPrivateGlobalPrefix() const {
>> if (hasLinkerPrivateGlobalPrefix())
>> return LinkerPrivateGlobalPrefix;
>> return getPrivateGlobalPrefix();
>>
>> Modified: llvm/trunk/include/llvm/MC/MCInstrInfo.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCInstrInfo.h?rev=283018&r1=283017&r2=283018&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCInstrInfo.h?rev=283018&r1=283017&r2=283018&view=diff>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/MC/MCInstrInfo.h (original)
>> +++ llvm/trunk/include/llvm/MC/MCInstrInfo.h Sat Oct 1 01:46:33 2016
>> @@ -48,9 +48,9 @@ public:
>> }
>>
>> /// \brief Returns the name for the instructions with the given opcode.
>> - const char *getName(unsigned Opcode) const {
>> + StringRef getName(unsigned Opcode) const {
>> assert(Opcode < NumOpcodes && "Invalid opcode!");
>> - return &InstrNameData[InstrNameIndices[Opcode]];
>> + return StringRef(&InstrNameData[InstrNameIndices[Opcode]]);
>> }
>> };
>>
>>
>> Modified: llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp?rev=283018&r1=283017&r2=283018&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp?rev=283018&r1=283017&r2=283018&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp Sat Oct 1 01:46:33 2016
>> @@ -51,7 +51,7 @@ MCSymbol *MachineBasicBlock::getSymbol()
>> if (!CachedMCSymbol) {
>> const MachineFunction *MF = getParent();
>> MCContext &Ctx = MF->getContext();
>> - const char *Prefix = Ctx.getAsmInfo()->getPrivateLabelPrefix();
>> + auto Prefix = Ctx.getAsmInfo()->getPrivateLabelPrefix();
>> assert(getNumber() >= 0 && "cannot get label for unreachable MBB");
>> CachedMCSymbol = Ctx.getOrCreateSymbol(Twine(Prefix) + "BB" +
>> Twine(MF->getFunctionNumber()) +
>>
>> Modified: llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp?rev=283018&r1=283017&r2=283018&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp?rev=283018&r1=283017&r2=283018&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp Sat Oct 1 01:46:33 2016
>> @@ -84,8 +84,8 @@ unsigned TargetInstrInfo::getInlineAsmLe
>> if (*Str == '\n' || strncmp(Str, MAI.getSeparatorString(),
>> strlen(MAI.getSeparatorString())) == 0) {
>> atInsnStart = true;
>> - } else if (strncmp(Str, MAI.getCommentString(),
>> - strlen(MAI.getCommentString())) == 0) {
>> + } else if (strncmp(Str, MAI.getCommentString().data(),
>> + MAI.getCommentString().size()) == 0) {
>
>> This should be "MAI.getCommentString() == Str”
>
> This implies building a StringRef with Str which means that 1) you need to be sure that Str is null terminated, and 2) you’ll run strlen on Str before doing a str compare, and 3) the most important, the original comparison matches that Str *starts with* getCommentString() while your replacement is an exact match.
> So the correct replacement would be `StringRef(Str).startwith(MAI.getCommentString())`, with the two drawbacks above.
> Yea you are right. In that case, how hard is it to change Str to a StringRef as well?
>
>>
>> Are all these strings guaranteed to be null terminated? I see a lot of calling .data()
>
> Since I’m converting from raw pointer, I assume that they were till now, otherwise it would have crashed? Maybe I missed something?
> Yea I suppose so. I just get worried when I see .data(). For example the following is unsafe:
>
> void Foo(StringRef S) {
> const char *cstr = S.data();
> }
>
> void Bar() {
> Foo("Test");
> }
>
> Because while it is safe from Bar, you don't know who else might be calling Foo. As long as you control all inputs to Foo though it's ok. To avoid this, whenever I convert something, I've been converting everything else that depends on it too, insofar as it's practical to do it without exploding into a million file CL.
Right, I still have over 20 patches in my queue, trying to propagate StringRef over the codebase. I’m validating them one-by-one right now (upgrading clang/lldb/etc. at the same time).
—
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161001/4ed26aef/attachment.html>
More information about the llvm-commits
mailing list