[PATCH] D36266: [ELF] - Remove ScriptLexer::Error field and check ErrorCount instead.

[Rafael Avila de Espindola via llvm-commits]

LGTM

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar created this revision.
> Herald added a subscriber: emaste.
>
> https://reviews.llvm.org/D35945 introduces change when there is useless to check `Error` flag
> in few places, but ErrorCount must be checked instead.
>
> But then we probably can just check ErrorCount always. That should simplify
> things. Patch do that.
>
>
> https://reviews.llvm.org/D36266
>
> Files:
>   ELF/ScriptLexer.cpp
>   ELF/ScriptLexer.h
>   ELF/ScriptParser.cpp
>   test/ELF/version-script-err.s
>
> Index: test/ELF/version-script-err.s
> ===================================================================
> --- test/ELF/version-script-err.s
> +++ test/ELF/version-script-err.s
> @@ -8,4 +8,3 @@
>  // RUN: not ld.lld --version-script %terr1.script -shared %t.o -o %t.so 2>&1 | \
>  // RUN:   FileCheck -check-prefix=ERR1 %s
>  // ERR1: {{.*}}:1: unclosed quote
> -// ERR1-NEXT: {{.*}}: unexpected EOF
> Index: ELF/ScriptParser.cpp
> ===================================================================
> --- ELF/ScriptParser.cpp
> +++ ELF/ScriptParser.cpp
> @@ -190,7 +190,7 @@
>      return;
>    }
>  
> -  while (!atEOF() && !Error && peek() != "}") {
> +  while (!atEOF() && !ErrorCount && peek() != "}") {
>      StringRef VerStr = next();
>      if (VerStr == "{") {
>        setError("anonymous version definition is used in "
> @@ -282,7 +282,7 @@
>    expect("(");
>    bool Orig = Config->AsNeeded;
>    Config->AsNeeded = true;
> -  while (!Error && !consume(")"))
> +  while (!ErrorCount && !consume(")"))
>      addFile(unquote(next()));
>    Config->AsNeeded = Orig;
>  }
> @@ -298,13 +298,13 @@
>  
>  void ScriptParser::readExtern() {
>    expect("(");
> -  while (!Error && !consume(")"))
> +  while (!ErrorCount && !consume(")"))
>      Config->Undefined.push_back(next());
>  }
>  
>  void ScriptParser::readGroup() {
>    expect("(");
> -  while (!Error && !consume(")")) {
> +  while (!ErrorCount && !consume(")")) {
>      if (consume("AS_NEEDED"))
>        readAsNeeded();
>      else
> @@ -343,7 +343,7 @@
>  void ScriptParser::readOutputArch() {
>    // OUTPUT_ARCH is ignored for now.
>    expect("(");
> -  while (!Error && !consume(")"))
> +  while (!ErrorCount && !consume(")"))
>      skip();
>  }
>  
> @@ -362,14 +362,14 @@
>  
>  void ScriptParser::readPhdrs() {
>    expect("{");
> -  while (!Error && !consume("}")) {
> +  while (!ErrorCount && !consume("}")) {
>      Script->Opt.PhdrsCommands.push_back(
>          {next(), PT_NULL, false, false, UINT_MAX, nullptr});
>  
>      PhdrsCommand &PhdrCmd = Script->Opt.PhdrsCommands.back();
>      PhdrCmd.Type = readPhdrType();
>  
> -    while (!Error && !consume(";")) {
> +    while (!ErrorCount && !consume(";")) {
>        if (consume("FILEHDR"))
>          PhdrCmd.HasFilehdr = true;
>        else if (consume("PHDRS"))
> @@ -401,7 +401,7 @@
>    Config->SingleRoRx = true;
>  
>    expect("{");
> -  while (!Error && !consume("}")) {
> +  while (!ErrorCount && !consume("}")) {
>      StringRef Tok = next();
>      BaseCommand *Cmd = readProvideOrAssignment(Tok);
>      if (!Cmd) {
> @@ -426,7 +426,7 @@
>  
>  StringMatcher ScriptParser::readFilePatterns() {
>    std::vector<StringRef> V;
> -  while (!Error && !consume(")"))
> +  while (!ErrorCount && !consume(")"))
>      V.push_back(next());
>    return StringMatcher(V);
>  }
> @@ -458,15 +458,15 @@
>  // any file but a.o, and section .baz in any file but b.o.
>  std::vector<SectionPattern> ScriptParser::readInputSectionsList() {
>    std::vector<SectionPattern> Ret;
> -  while (!Error && peek() != ")") {
> +  while (!ErrorCount && peek() != ")") {
>      StringMatcher ExcludeFilePat;
>      if (consume("EXCLUDE_FILE")) {
>        expect("(");
>        ExcludeFilePat = readFilePatterns();
>      }
>  
>      std::vector<StringRef> V;
> -    while (!Error && peek() != ")" && peek() != "EXCLUDE_FILE")
> +    while (!ErrorCount && peek() != ")" && peek() != "EXCLUDE_FILE")
>        V.push_back(next());
>  
>      if (!V.empty())
> @@ -493,7 +493,7 @@
>    auto *Cmd = make<InputSectionDescription>(FilePattern);
>    expect("(");
>  
> -  while (!Error && !consume(")")) {
> +  while (!ErrorCount && !consume(")")) {
>      SortSectionPolicy Outer = readSortKind();
>      SortSectionPolicy Inner = SortSectionPolicy::Default;
>      std::vector<SectionPattern> V;
> @@ -623,7 +623,7 @@
>      Cmd->Constraint = ConstraintKind::ReadWrite;
>    expect("{");
>  
> -  while (!Error && !consume("}")) {
> +  while (!ErrorCount && !consume("}")) {
>      StringRef Tok = next();
>      if (Tok == ";") {
>        // Empty commands are allowed. Do nothing here.
> @@ -767,7 +767,7 @@
>  // This is a part of the operator-precedence parser. This function
>  // assumes that the remaining token stream starts with an operator.
>  Expr ScriptParser::readExpr1(Expr Lhs, int MinPrec) {
> -  while (!atEOF() && !Error) {
> +  while (!atEOF() && !ErrorCount) {
>      // Read an operator and an expression.
>      if (consume("?"))
>        return readTernary(Lhs);
> @@ -1021,7 +1021,7 @@
>  
>  std::vector<StringRef> ScriptParser::readOutputSectionPhdrs() {
>    std::vector<StringRef> Phdrs;
> -  while (!Error && peek().startswith(":")) {
> +  while (!ErrorCount && peek().startswith(":")) {
>      StringRef Tok = next();
>      Phdrs.push_back((Tok.size() == 1) ? next() : Tok.substr(1));
>    }
> @@ -1124,7 +1124,7 @@
>    std::vector<SymbolVersion> Globals;
>    std::vector<SymbolVersion> *V = &Globals;
>  
> -  while (!Error) {
> +  while (!ErrorCount) {
>      if (consume("}"))
>        break;
>      if (consumeLabel("local")) {
> @@ -1158,7 +1158,7 @@
>    expect("{");
>  
>    std::vector<SymbolVersion> Ret;
> -  while (!Error && peek() != "}") {
> +  while (!ErrorCount && peek() != "}") {
>      StringRef Tok = next();
>      bool HasWildcard = !Tok.startswith("\"") && hasWildcard(Tok);
>      Ret.push_back({unquote(Tok), IsCXX, HasWildcard});
> @@ -1185,7 +1185,7 @@
>  // MEMORY { name [(attr)] : ORIGIN = origin, LENGTH = len ... }
>  void ScriptParser::readMemory() {
>    expect("{");
> -  while (!Error && !consume("}")) {
> +  while (!ErrorCount && !consume("}")) {
>      StringRef Name = next();
>  
>      uint32_t Flags = 0;
> Index: ELF/ScriptLexer.h
> ===================================================================
> --- ELF/ScriptLexer.h
> +++ ELF/ScriptLexer.h
> @@ -39,7 +39,6 @@
>    std::vector<StringRef> Tokens;
>    bool InExpr = false;
>    size_t Pos = 0;
> -  bool Error = false;
>  
>  private:
>    void maybeSplitExpr();
> Index: ELF/ScriptLexer.cpp
> ===================================================================
> --- ELF/ScriptLexer.cpp
> +++ ELF/ScriptLexer.cpp
> @@ -75,9 +75,8 @@
>  
>  // We don't want to record cascading errors. Keep only the first one.
>  void ScriptLexer::setError(const Twine &Msg) {
> -  if (Error)
> +  if (ErrorCount)
>      return;
> -  Error = true;
>  
>    if (!Pos) {
>      error(getCurrentLocation() + ": " + Msg);
> @@ -164,7 +163,7 @@
>  }
>  
>  // An erroneous token is handled as if it were the last token before EOF.
> -bool ScriptLexer::atEOF() { return Error || Tokens.size() == Pos; }
> +bool ScriptLexer::atEOF() { return ErrorCount || Tokens.size() == Pos; }
>  
>  // Split a given string as an expression.
>  // This function returns "3", "*" and "5" for "3*5" for example.
> @@ -207,7 +206,7 @@
>  //
>  // This function may split the current token into multiple tokens.
>  void ScriptLexer::maybeSplitExpr() {
> -  if (!InExpr || Error || atEOF())
> +  if (!InExpr || ErrorCount || atEOF())
>      return;
>  
>    std::vector<StringRef> V = tokenizeExpr(Tokens[Pos]);
> @@ -220,7 +219,7 @@
>  StringRef ScriptLexer::next() {
>    maybeSplitExpr();
>  
> -  if (Error)
> +  if (ErrorCount)
>      return "";
>    if (atEOF()) {
>      setError("unexpected EOF");
> @@ -231,7 +230,7 @@
>  
>  StringRef ScriptLexer::peek() {
>    StringRef Tok = next();
> -  if (Error)
> +  if (ErrorCount)
>      return "";
>    Pos = Pos - 1;
>    return Tok;
> @@ -260,7 +259,7 @@
>  void ScriptLexer::skip() { (void)next(); }
>  
>  void ScriptLexer::expect(StringRef Expect) {
> -  if (Error)
> +  if (ErrorCount)
>      return;
>    StringRef Tok = next();
>    if (Tok != Expect)