[llvm] r294555 - [GlobalISel] Simplify StringRef parameters. NFC.

[Ahmed Bougacha via llvm-commits]

On Thu, Feb 9, 2017 at 1:32 AM, Daniel Sanders
<daniel_l_sanders at apple.com> wrote:
>> 'const' on StringRef parameters adds no guarantees. Remove it.
>
> This is incorrect, this const guarantees that the function does not allow assignment to that variable. The string itself is constant in both cases but 'StringRef' can be locally re-assigned to another string whereas 'const StringRef' cannot.

Sure, but that's a guarantee you impose yourself in the
implementation, not a guarantee that belongs in the declared API.
>From the API standpoint, that const doesn't prevent the function from
having a local copy of the StringRef, which would have the exact same
effect.

Now, putting it in the various implementations is more subjective, but
there a different argument can be made: the functions are all 2 to 3
lines, and it's obvious they don't modify it.

But I don't feel strongly either way, this is just a very unusual thing to see.

-Ahmed

> It was my intention to ensure that the InsnVarName always referred to the string provided by the caller so I'd like you to revert this unless there's a good reason to make InsnVarName assignable.
>
>> On 9 Feb 2017, at 02:50, Ahmed Bougacha via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: ab
>> Date: Wed Feb  8 20:50:01 2017
>> New Revision: 294555
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=294555&view=rev
>> Log:
>> [GlobalISel] Simplify StringRef parameters. NFC.
>>
>> 'const' on StringRef parameters adds no guarantees. Remove it.
>>
>> Modified:
>>    llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp
>>
>> Modified: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp?rev=294555&r1=294554&r2=294555&view=diff
>> ==============================================================================
>> --- llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp (original)
>> +++ llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp Wed Feb  8 20:50:01 2017
>> @@ -160,8 +160,7 @@ public:
>>
>>   /// Emit a C++ expression that checks the predicate for the OpIdx operand of
>>   /// the instruction given in InsnVarName.
>> -  virtual void emitCxxPredicateExpr(raw_ostream &OS,
>> -                                    const StringRef InsnVarName,
>> +  virtual void emitCxxPredicateExpr(raw_ostream &OS, StringRef InsnVarName,
>>                                     unsigned OpIdx) const = 0;
>> };
>>
>> @@ -173,7 +172,7 @@ protected:
>> public:
>>   LLTOperandMatcher(std::string Ty) : Ty(Ty) {}
>>
>> -  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName,
>> +  void emitCxxPredicateExpr(raw_ostream &OS, StringRef InsnVarName,
>>                             unsigned OpIdx) const override {
>>     OS << "MRI.getType(" << InsnVarName << ".getOperand(" << OpIdx
>>        << ").getReg()) == (" << Ty << ")";
>> @@ -188,7 +187,7 @@ protected:
>> public:
>>   RegisterBankOperandMatcher(const CodeGenRegisterClass &RC) : RC(RC) {}
>>
>> -  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName,
>> +  void emitCxxPredicateExpr(raw_ostream &OS, StringRef InsnVarName,
>>                             unsigned OpIdx) const override {
>>     OS << "(&RBI.getRegBankFromRegClass(" << RC.getQualifiedName()
>>        << "RegClass) == RBI.getRegBank(" << InsnVarName << ".getOperand("
>> @@ -199,7 +198,7 @@ public:
>> /// Generates code to check that an operand is a basic block.
>> class MBBOperandMatcher : public OperandPredicateMatcher {
>> public:
>> -  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName,
>> +  void emitCxxPredicateExpr(raw_ostream &OS, StringRef InsnVarName,
>>                             unsigned OpIdx) const override {
>>     OS << InsnVarName << ".getOperand(" << OpIdx << ").isMBB()";
>>   }
>> @@ -216,7 +215,7 @@ public:
>>
>>   /// Emit a C++ expression that tests whether the instruction named in
>>   /// InsnVarName matches all the predicate and all the operands.
>> -  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName) const {
>> +  void emitCxxPredicateExpr(raw_ostream &OS, StringRef InsnVarName) const {
>>     OS << "(";
>>     emitCxxPredicateListExpr(OS, InsnVarName, OpIdx);
>>     OS << ")";
>> @@ -235,7 +234,7 @@ public:
>>   /// Emit a C++ expression that tests whether the instruction named in
>>   /// InsnVarName matches the predicate.
>>   virtual void emitCxxPredicateExpr(raw_ostream &OS,
>> -                                    const StringRef InsnVarName) const = 0;
>> +                                    StringRef InsnVarName) const = 0;
>> };
>>
>> /// Generates code to check the opcode of an instruction.
>> @@ -247,7 +246,7 @@ public:
>>   InstructionOpcodeMatcher(const CodeGenInstruction *I) : I(I) {}
>>
>>   void emitCxxPredicateExpr(raw_ostream &OS,
>> -                            const StringRef InsnVarName) const override {
>> +                            StringRef InsnVarName) const override {
>>     OS << InsnVarName << ".getOpcode() == " << I->Namespace
>>        << "::" << I->TheDef->getName();
>>   }
>> @@ -273,7 +272,7 @@ public:
>>
>>   /// Emit a C++ expression that tests whether the instruction named in
>>   /// InsnVarName matches all the predicates and all the operands.
>> -  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName) const {
>> +  void emitCxxPredicateExpr(raw_ostream &OS, StringRef InsnVarName) const {
>>     emitCxxPredicateListExpr(OS, InsnVarName);
>>     for (const auto &Operand : Operands) {
>>       OS << " &&\n(";
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>