[llvm] r294555 - [GlobalISel] Simplify StringRef parameters. NFC.
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 9 09:19:41 PST 2017
On 9 Feb 2017, at 15:43, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
>
> 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.
Yes, it's only relevant to the implementation right now (but that's still useful). On most objects there would also be an an API-level guarantee that calls to non-const member functions aren't allowed but there aren't any defined in StringRef at the moment so that guarantee is moot at the moment (but might not be in the future).
By the way, I think these ought to be 'const StringRef &'. It's more common and also avoids copying the StringRef object.
> 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.
That's a fair point but I'd prefer the code to say it's not supposed to be re-assigned.
> 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
>>
More information about the llvm-commits
mailing list