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

[Daniel Sanders via llvm-commits]

> On 9 Feb 2017, at 20:21, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
> For instance why not do it on every single value arguments? Would you do it on a integer passed by value?

In general no. Mostly because of tradition rather than reason.

>> 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

I don't think it's worth going into a big debate on llvm-dev about it. I personally prefer to const object parameters where possible but if you both feel that StringRef is never const then that's fair enough.

>>> 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
>> 
>