MIR Parser: Simplify the token's string value handling.

[arphaman]

Summary

This patch removes the StringOffset and HasStringValue fields from the MIToken struct and simplifies the stringValue method which now returns the new StringValue field.

[silvas]

Except for removing rawStringValue this entire suggestion was pretty nitpicky anyway, so feel free to ignore if any of this doesn't seem worth it.

lib/CodeGen/MIRParser/MILexer.h

113

It seems weird to me that we are adding an operator= (and a non-obvious one at that) for a class that is never really being assigned. It seems like the complexity is coming from the use of MIToken constructor to reinitialize an MIToken. I think it would be simpler to use two explicit calls:

• resetToken(Kind, Range)

then one of

• setIntVal(APSInt &)
• setStrValNonOwned(StringRef) (or use an offset; but I like the symmetry with Owned)
• setStrValOwned(StringRef/std::string) (copies into internal buffer and sets StrVal)

Conveniently all the existing uses of the MIToken constructors like this are already broken across two lines, so this should not increase the size of the reinitialization source code (but allows deleting code duplicated across these 3 constructors).

An even simpler solution is to just use std::string/SmallString and have it always own (not sure how performance-critical this is). Once it grows large enough there should be no further need to reallocate, so it is just a small memcpy cost.

[arphaman]

The updated patch now uses the 'reset' and 'set...' pattern instead of MIToken's constructors as suggested by Sean.

[silvas]

Nice! I wasn't expecting a builder pattern but that turns out really nice! LGTM

[silvas]

Mark as "Accept" in Phab. (don't think it really matters, but just to be consistent with the LGTM in the previous comment).

lib/CodeGen/MIRParser/MILexer.h

enum TokenKind {
     JumpTableIndex,
     NamedIRBlock,
     IRBlock,
     NamedIRValue,
   };
 
 private:
   TokenKind Kind;
-  unsigned StringOffset;
-  bool HasStringValue;
   StringRef Range;
-  std::string StringValue;
+  StringRef StringValue;
+  std::string StringValueStorage;
   APSInt IntVal;
 
 public:
   MIToken(TokenKind Kind, StringRef Range, unsigned StringOffset = 0)
-      : Kind(Kind), StringOffset(StringOffset), HasStringValue(false),
-        Range(Range) {}
+      : Kind(Kind), Range(Range), StringValue(Range.drop_front(StringOffset)) {}
 
-  MIToken(TokenKind Kind, StringRef Range, std::string StringValue,
-          unsigned StringOffset = 0)
-      : Kind(Kind), StringOffset(StringOffset), HasStringValue(true),
-        Range(Range), StringValue(std::move(StringValue)) {}
+  MIToken(TokenKind Kind, StringRef Range, std::string StrVal)
+      : Kind(Kind), Range(Range), StringValueStorage(std::move(StrVal)) {
+    StringValue = StringValueStorage;
+  }
 
   MIToken(TokenKind Kind, StringRef Range, const APSInt &IntVal,
           unsigned StringOffset = 0)
-      : Kind(Kind), StringOffset(StringOffset), HasStringValue(false),
-        Range(Range), IntVal(IntVal) {}
+      : Kind(Kind), Range(Range), StringValue(Range.drop_front(StringOffset)),
+        IntVal(IntVal) {}
+
+  MIToken &operator=(MIToken &&Other) {
+    Kind = Other.Kind;
+    Range = std::move(Other.Range);
+    StringValueStorage = std::move(Other.StringValueStorage);
+    if (StringValueStorage.empty())
+      StringValue = std::move(Other.StringValue);
+    else {
+      StringValue = StringValueStorage;
+      Other.StringValue = "";
+    }
+    IntVal = std::move(Other.IntVal);
+    return *this;
+  }
 
   TokenKind kind() const { return Kind; }
 
   bool isError() const { return Kind == Error; }
 
   bool isRegister() const {
     return Kind == NamedRegister || Kind == underscore ||
            Kind == VirtualRegister;
   }
 
   bool isRegisterFlag() const {
     return Kind == kw_implicit || Kind == kw_implicit_define ||
            Kind == kw_dead || Kind == kw_killed || Kind == kw_undef ||
            Kind == kw_early_clobber || Kind == kw_debug_use;
   }
 
   bool isMemoryOperandFlag() const { return Kind == kw_volatile; }
 
   bool is(TokenKind K) const { return Kind == K; }
 
   bool isNot(TokenKind K) const { return Kind != K; }
 
   StringRef::iterator location() const { return Range.begin(); }
 
-  /// Return the token's raw string value.
-  ///
-  /// If the string value is quoted, this method returns that quoted string as
-  /// it is, without unescaping the string value.
-  StringRef rawStringValue() const { return Range.drop_front(StringOffset); }
+  StringRef range() const { return Range; }
 
   /// Return the token's string value.
-  StringRef stringValue() const {
-    return HasStringValue ? StringRef(StringValue)
-                          : Range.drop_front(StringOffset);
-  }
+  StringRef stringValue() const { return StringValue; }
 
   const APSInt &integerValue() const { return IntVal; }
 
   bool hasIntegerValue() const {
     return Kind == IntegerLiteral || Kind == MachineBasicBlock ||
            Kind == StackObject || Kind == FixedStackObject ||
            Kind == GlobalValue || Kind == VirtualRegister ||
            Kind == ConstantPoolItem || Kind == JumpTableIndex ||

lib/CodeGen/MIRParser/MILexer.cpp

static Cursor lexName(
     Cursor C, MIToken &Token, MIToken::TokenKind Type, unsigned PrefixLength,
     function_ref<void(StringRef::iterator Loc, const Twine &)> ErrorCallback) {
   auto Range = C;
   C.advance(PrefixLength);
   if (C.peek() == '"') {
     if (Cursor R = lexStringConstant(C, ErrorCallback)) {
       StringRef String = Range.upto(R);
       Token = MIToken(Type, String,
-                      unescapeQuotedString(String.drop_front(PrefixLength)),
-                      PrefixLength);
+                      unescapeQuotedString(String.drop_front(PrefixLength)));
       return R;
     }
     Token = MIToken(MIToken::Error, Range.remaining());
     return Range;
   }
   while (isIdentifierChar(C.peek()))
     C.advance();
   Token = MIToken(Type, Range.upto(C), PrefixLength);

lib/CodeGen/MIRParser/MIParser.cpp

 }
 
 bool MIParser::parseGlobalValue(GlobalValue *&GV) {
   switch (Token.kind()) {
   case MIToken::NamedGlobalValue: {
     const Module *M = MF.getFunction()->getParent();
     GV = M->getNamedValue(Token.stringValue());
     if (!GV)
-      return error(Twine("use of undefined global value '@") +
-                   Token.rawStringValue() + "'");
+      return error(Twine("use of undefined global value '") + Token.range() +
+                   "'");
     break;
   }
   case MIToken::GlobalValue: {
     unsigned GVIdx;
     if (getUnsigned(GVIdx))
       return true;
     if (GVIdx >= IRSlots.GlobalValues.size())
       return error(Twine("use of undefined global value '@") + Twine(GVIdx) +

 }
 
 bool MIParser::parseIRBlock(BasicBlock *&BB, const Function &F) {
   switch (Token.kind()) {
   case MIToken::NamedIRBlock: {
     BB = dyn_cast_or_null<BasicBlock>(
         F.getValueSymbolTable().lookup(Token.stringValue()));
     if (!BB)
-      return error(Twine("use of undefined IR block '%ir-block.") +
-                   Token.rawStringValue() + "'");
+      return error(Twine("use of undefined IR block '") + Token.range() + "'");
     break;
   }
   case MIToken::IRBlock: {
     unsigned SlotNumber = 0;
     if (getUnsigned(SlotNumber))
       return true;
     BB = const_cast<BasicBlock *>(getIRBlock(SlotNumber));
     if (!BB)
bool MIParser::parseOperandsOffset(MachineOperand &Op) {
   return false;
 }
 
 bool MIParser::parseIRValue(Value *&V) {
   switch (Token.kind()) {
   case MIToken::NamedIRValue: {
     V = MF.getFunction()->getValueSymbolTable().lookup(Token.stringValue());
     if (!V)
-      return error(Twine("use of undefined IR value '%ir.") +
-                   Token.rawStringValue() + "'");
+      return error(Twine("use of undefined IR value '") + Token.range() + "'");
     break;
   }
   // TODO: Parse unnamed IR value references.
   default:
     llvm_unreachable("The current token should be an IR block reference");
   }
   return false;
 }