Simplify error reporting in YAMLParser

[chfast]

Summary

This change removes Location param in setError() functions. It was never used.

Alternative to https://reviews.llvm.org/D22565

[Bigcheese]

There are some cases where it looks like the argument isn't the same as Current. Is this actually the case?

lib/Support/YAMLParser.cpp

998–999

This one appears to change behavior with this patch. I don't believe this will ever be the same token as Current.

[chfast]

lib/Support/YAMLParser.cpp

998–999

You are partly right. The information provided by the second argument is never used because the param in setError() function is unused. This is the true origin of this patch.

I've changes that in https://reviews.llvm.org/D22565. But apparently nobody cares about this detail in error reports as this has been unnoticed for years. So this patch keeps status quo and simplifies the code.

[chfast]

Ping.

[chfast]

Ping².

[chfast]

As you are doing something with YAML, can you take a look here?

include/llvm/Support/YAMLParser.h

public:
   SMRange getSourceRange() const { return SourceRange; }
   void setSourceRange(SMRange SR) { SourceRange = SR; }
 
   // These functions forward to Document and Scanner.
   Token &peekNext();
   Token getNext();
   Node *parseBlockNode();
   BumpPtrAllocator &getAllocator();
-  void setError(const Twine &Message, Token &Location) const;
+  void setError(const Twine &Message) const;
   bool failed() const;
 
   virtual void skip() {}
 
   unsigned int getType() const { return TypeID; }
 
   void *operator new(size_t Size, BumpPtrAllocator &Alloc,
                      size_t Alignment = 16) LLVM_NOEXCEPT {
private:
   ///        document.
   Node *Root;
 
   /// \brief Maps tag prefixes to their expansion.
   std::map<StringRef, StringRef> TagMap;
 
   Token &peekNext();
   Token getNext();
-  void setError(const Twine &Message, Token &Location) const;
+  void setError(const Twine &Message) const;
   bool failed() const;
 
   /// \brief Parse %BLAH directives and return true if any were encountered.
   bool parseDirectives();
 
   /// \brief Parse %YAML
   void parseYAMLDirective();
 

lib/Support/YAMLParser.cpp

public:
   /// @brief Parse the next token and pop it from the queue.
   Token getNext();
 
   void printError(SMLoc Loc, SourceMgr::DiagKind Kind, const Twine &Message,
                   ArrayRef<SMRange> Ranges = None) {
     SM.PrintMessage(Loc, Kind, Message, Ranges, /* FixIts= */ None, ShowColors);
   }
 
-  void setError(const Twine &Message, StringRef::iterator Position) {
+  void setError(const Twine &Message) {
     if (Current >= End)
       Current = End - 1;
 
     // Don't print out more errors after the first one we encounter. The rest
     // are just the result of the first, and have no meaning.
     if (!Failed)
       printError(SMLoc::getFromPointer(Current), SourceMgr::DK_Error, Message);
     Failed = true;
   }
 
-  void setError(const Twine &Message) {
-    setError(Message, Current);
-  }
-
   /// @brief Returns true if an error occurred while parsing.
   bool failed() {
     return Failed;
   }
 
 private:
   void init(MemoryBufferRef Buffer);
 
void Scanner::saveSimpleKeyCandidate( TokenQueueT::iterator Tok
   }
 }
 
 void Scanner::removeStaleSimpleKeyCandidates() {
   for (SmallVectorImpl<SimpleKey>::iterator i = SimpleKeys.begin();
                                             i != SimpleKeys.end();) {
     if (i->Line != Line || i->Column + 1024 < Column) {
       if (i->IsRequired)
-        setError( "Could not find expected : for simple key"
-                , i->Tok->Range.begin());
+        setError("Could not find expected : for simple key");
       i = SimpleKeys.erase(i);
     } else
       ++i;
   }
 }
 
 void Scanner::removeSimpleKeyCandidatesOnFlowLevel(unsigned Level) {
   if (!SimpleKeys.empty() && (SimpleKeys.end() - 1)->FlowLevel == Level)
while (true) {
           break;
         Current = i;
         ++Column;
       }
     }
   }
 
   if (Current == End) {
-    setError("Expected quote at end of scalar", Current);
+    setError("Expected quote at end of scalar");
     return false;
   }
 
   skip(1); // Skip ending quote.
   Token T;
   T.Kind = Token::TK_Scalar;
   T.Range = StringRef(Start, Current - Start);
   TokenQueue.push_back(T);
bool Scanner::scanPlainScalar() {
   unsigned indent = static_cast<unsigned>(Indent + 1);
   while (true) {
     if (*Current == '#')
       break;
 
     while (!isBlankOrBreak(Current)) {
       if (  FlowLevel && *Current == ':'
           && !(isBlankOrBreak(Current + 1) || *(Current + 1) == ',')) {
-        setError("Found unexpected ':' while scanning a plain scalar", Current);
+        setError("Found unexpected ':' while scanning a plain scalar");
         return false;
       }
 
       // Check for the end of the plain scalar.
       if (  (*Current == ':' && isBlankOrBreak(Current + 1))
           || (  FlowLevel
           && (StringRef(Current, 1).find_first_of(",:?[]{}")
               != StringRef::npos)))
if (!isBlankOrBreak(Current))
       break;
 
     // Eat blanks.
     StringRef::iterator Tmp = Current;
     while (isBlankOrBreak(Tmp)) {
       StringRef::iterator i = skip_s_white(Tmp);
       if (i != Tmp) {
         if (LeadingBlanks && (Column < indent) && *Tmp == '\t') {
-          setError("Found invalid tab character in indentation", Tmp);
+          setError("Found invalid tab character in indentation");
           return false;
         }
         Tmp = i;
         ++Column;
       } else {
         i = skip_b_break(Tmp);
         if (!LeadingBlanks)
           LeadingBlanks = 1;
         Tmp = i;
         Column = 0;
         ++Line;
       }
     }
 
     if (!FlowLevel && Column < indent)
       break;
 
     Current = Tmp;
   }
   if (Start == Current) {
-    setError("Got empty plain scalar", Start);
+    setError("Got empty plain scalar");
     return false;
   }
   Token T;
   T.Kind = Token::TK_Scalar;
   T.Range = StringRef(Start, Current - Start);
   TokenQueue.push_back(T);
 
   // Plain scalars can be simple keys.
while(true) {
     StringRef::iterator i = skip_ns_char(Current);
     if (i == Current)
       break;
     Current = i;
     ++Column;
   }
 
   if (Start == Current) {
-    setError("Got empty alias or anchor", Start);
+    setError("Got empty alias or anchor");
     return false;
   }
 
   Token T;
   T.Kind = IsAlias ? Token::TK_Alias : Token::TK_Anchor;
   T.Range = StringRef(Start, Current - Start);
   TokenQueue.push_back(T);
 
if (Current == End) { // EOF, we have an empty scalar.
     T.Kind = Token::TK_BlockScalar;
     T.Range = StringRef(Start, Current - Start);
     TokenQueue.push_back(T);
     IsDone = true;
     return true;
   }
 
   if (!consumeLineBreakIfPresent()) {
-    setError("Expected a line break after block scalar header", Current);
+    setError("Expected a line break after block scalar header");
     return false;
   }
   return true;
 }
 
 bool Scanner::findBlockScalarIndent(unsigned &BlockIndent,
                                     unsigned BlockExitIndent,
                                     unsigned &LineBreaks, bool &IsDone) {
   unsigned MaxAllSpaceLineCharacters = 0;
-  StringRef::iterator LongestAllSpaceLine;
 
   while (true) {
     advanceWhile(&Scanner::skip_s_space);
     if (skip_nb_char(Current) != Current) {
       // This line isn't empty, so try and find the indentation.
       if (Column <= BlockExitIndent) { // End of the block literal.
         IsDone = true;
         return true;
       }
       // We found the block's indentation.
       BlockIndent = Column;
       if (MaxAllSpaceLineCharacters > BlockIndent) {
         setError(
-            "Leading all-spaces line must be smaller than the block indent",
-            LongestAllSpaceLine);
+            "Leading all-spaces line must be smaller than the block indent");
         return false;
       }
       return true;
     }
     if (skip_b_break(Current) != Current &&
         Column > MaxAllSpaceLineCharacters) {
       // Record the longest all-space line in case it's longer than the
       // discovered block indent.
       MaxAllSpaceLineCharacters = Column;
-      LongestAllSpaceLine = Current;
     }
 
     // Check for EOF.
     if (Current == End) {
       IsDone = true;
       return true;
     }
 
if (Column <= BlockExitIndent) { // End of the block literal.
     return true;
   }
 
   if (Column < BlockIndent) {
     if (Current != End && *Current == '#') { // Trailing comment.
       IsDone = true;
       return true;
     }
-    setError("A text line is less indented than the block scalar", Current);
+    setError("A text line is less indented than the block scalar");
     return false;
   }
   return true; // A normal text line.
 }
 
 bool Scanner::scanBlockScalar(bool IsLiteral) {
   // Eat '|' or '>'
   assert(*Current == '|' || *Current == '>');
if (Raw.find_last_of('!') == 0) {
       std::map<StringRef, StringRef>::const_iterator It =
           Doc->getTagMap().find(TagHandle);
       if (It != Doc->getTagMap().end())
         Ret = It->second;
       else {
         Token T;
         T.Kind = Token::TK_Tag;
         T.Range = TagHandle;
-        setError(Twine("Unknown tag handle ") + TagHandle, T);
+        setError(Twine("Unknown tag handle ") + TagHandle);
       }
       Ret += Raw.substr(Raw.find_last_of('!') + 1);
       return Ret;
     }
   }
 
   switch (getType()) {
   case NK_Null:

 Node *Node::parseBlockNode() {
   return Doc->parseBlockNode();
 }
 
 BumpPtrAllocator &Node::getAllocator() {
   return Doc->NodeAllocator;
 }
 
-void Node::setError(const Twine &Msg, Token &Tok) const {
-  Doc->setError(Msg, Tok);
+void Node::setError(const Twine &Msg) const {
+  Doc->setError(Msg);
 }
 
 bool Node::failed() const {
   return Doc->failed();
 }
 
 
 
default:
       if (UnquotedValue.size() == 1)
         // TODO: Report error.
         break;
       UnquotedValue = UnquotedValue.substr(1);
       switch (UnquotedValue[0]) {
       default: {
           Token T;
           T.Range = StringRef(UnquotedValue.begin(), 1);
-          setError("Unrecognized escape code!", T);
+          setError("Unrecognized escape code!");
           return "";
         }
       case '\r':
       case '\n':
         // Remove the new line.
         if (   UnquotedValue.size() > 1
             && (UnquotedValue[1] == '\r' || UnquotedValue[1] == '\n'))
           UnquotedValue = UnquotedValue.substr(1);
if (   t.Kind == Token::TK_BlockEnd
         || t.Kind == Token::TK_FlowMappingEnd
         || t.Kind == Token::TK_Key
         || t.Kind == Token::TK_FlowEntry
         || t.Kind == Token::TK_Error) {
       return Value = new (getAllocator()) NullNode(Doc);
     }
 
     if (t.Kind != Token::TK_Value) {
-      setError("Unexpected token in Key Value.", t);
+      setError("Unexpected token in Key Value.");
       return Value = new (getAllocator()) NullNode(Doc);
     }
     getNext(); // skip TK_Value.
   }
 
   // Handle explicit null values.
   Token &t = peekNext();
   if (t.Kind == Token::TK_BlockEnd || t.Kind == Token::TK_Key) {
void MappingNode::increment() {
   } else if (Type == MT_Block) {
     switch (T.Kind) {
     case Token::TK_BlockEnd:
       getNext();
       IsAtEnd = true;
       CurrentEntry = nullptr;
       break;
     default:
-      setError("Unexpected token. Expected Key or Block End", T);
+      setError("Unexpected token. Expected Key or Block End");
     case Token::TK_Error:
       IsAtEnd = true;
       CurrentEntry = nullptr;
     }
   } else {
     switch (T.Kind) {
     case Token::TK_FlowEntry:
       // Eat the flow entry and recurse.
       getNext();
       return increment();
     case Token::TK_FlowMappingEnd:
       getNext();
     case Token::TK_Error:
       // Set this to end iterator.
       IsAtEnd = true;
       CurrentEntry = nullptr;
       break;
     default:
-      setError( "Unexpected token. Expected Key, Flow Entry, or Flow "
-                "Mapping End."
-              , T);
+      setError(
+          "Unexpected token. Expected Key, Flow Entry, or Flow Mapping End.");
       IsAtEnd = true;
       CurrentEntry = nullptr;
     }
   }
 }
 
 void SequenceNode::increment() {
   if (failed()) {
case Token::TK_BlockEntry:
       }
       break;
     case Token::TK_BlockEnd:
       getNext();
       IsAtEnd = true;
       CurrentEntry = nullptr;
       break;
     default:
-      setError( "Unexpected token. Expected Block Entry or Block End."
-              , T);
+      setError("Unexpected token. Expected Block Entry or Block End.");
     case Token::TK_Error:
       IsAtEnd = true;
       CurrentEntry = nullptr;
     }
   } else if (SeqType == ST_Indentless) {
     switch (T.Kind) {
     case Token::TK_BlockEntry:
       getNext();
if (SeqType == ST_Block) {
     case Token::TK_Error:
       // Set this to end iterator.
       IsAtEnd = true;
       CurrentEntry = nullptr;
       break;
     case Token::TK_StreamEnd:
     case Token::TK_DocumentEnd:
     case Token::TK_DocumentStart:
-      setError("Could not find closing ]!", T);
+      setError("Could not find closing ]!");
       // Set this to end iterator.
       IsAtEnd = true;
       CurrentEntry = nullptr;
       break;
     default:
       if (!WasPreviousTokenFlowEntry) {
-        setError("Expected , between entries!", T);
+        setError("Expected , between entries!");
         IsAtEnd = true;
         CurrentEntry = nullptr;
         break;
       }
       // Otherwise it must be a flow entry.
       CurrentEntry = parseBlockNode();
       if (!CurrentEntry) {
         IsAtEnd = true;

 Token &Document::peekNext() {
   return stream.scanner->peekNext();
 }
 
 Token Document::getNext() {
   return stream.scanner->getNext();
 }
 
-void Document::setError(const Twine &Message, Token &Location) const {
-  stream.scanner->setError(Message, Location.Range.begin());
+void Document::setError(const Twine &Message) const {
+  stream.scanner->setError(Message);
 }
 
 bool Document::failed() const {
   return stream.scanner->failed();
 }
 
 Node *Document::parseBlockNode() {
   Token T = peekNext();
   // Handle properties.
   Token AnchorInfo;
   Token TagInfo;
 parse_property:
   switch (T.Kind) {
   case Token::TK_Alias:
     getNext();
     return new (NodeAllocator) AliasNode(stream.CurrentDoc, T.Range.substr(1));
   case Token::TK_Anchor:
     if (AnchorInfo.Kind == Token::TK_Anchor) {
-      setError("Already encountered an anchor for this node!", T);
+      setError("Already encountered an anchor for this node!");
       return nullptr;
     }
     AnchorInfo = getNext(); // Consume TK_Anchor.
     T = peekNext();
     goto parse_property;
   case Token::TK_Tag:
     if (TagInfo.Kind == Token::TK_Tag) {
-      setError("Already encountered a tag for this node!", T);
+      setError("Already encountered a tag for this node!");
       return nullptr;
     }
     TagInfo = getNext(); // Consume TK_Tag.
     T = peekNext();
     goto parse_property;
   default:
     break;
   }
void Document::parseTAGDirective() {
   StringRef TagHandle = T.substr(0, HandleEnd);
   StringRef TagPrefix = T.substr(HandleEnd).ltrim(" \t");
   TagMap[TagHandle] = TagPrefix;
 }
 
 bool Document::expectToken(int TK) {
   Token T = getNext();
   if (T.Kind != TK) {
-    setError("Unexpected token", T);
+    setError("Unexpected token");
     return false;
   }
   return true;
 }