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

[Mehdi Amini via llvm-commits]

For instance why not do it on every single value arguments? Would you do it on a integer passed by value?

> On Feb 9, 2017, at 12:18 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
> I can see how for long function, the ability to see that an argument is const allows to avoid tracking that its value may change in the body, so I sympathize with the intention, but if you’d like to push with this you should try to get the coding standard updated. Right now I don’t believe `const StringRef` is a common idiom in LLVM (I’ve never seen it before).
> 
> 
> 
> — 
> Mehdi
> 
> 
> 
>> On Feb 9, 2017, at 1:32 AM, Daniel Sanders via llvm-commits <llvm-commits at lists.llvm.org> 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.
>> 
>> 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
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>