[llvm] r294555 - [GlobalISel] Simplify StringRef parameters. NFC.
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 9 13:38:19 PST 2017
> On Feb 9, 2017, at 1:35 PM, Daniel Sanders <daniel_l_sanders at apple.com> wrote:
>
>
>> 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.
I agree with you, I was just pointing the existing practice. A few years ago I was trying to put const as much as possible and now I just go with the flow and focus on where it matters in the API :)
—
Mehdi
>
>>> 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
>>>
>>
>
More information about the llvm-commits
mailing list