[PATCH] D44560: [DWARF] Rework debug line parsing to use llvm::Error and callbacks

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

I hope the code snippet below helps clarify my original suggestion. Like I said earlier, this might be taking things a step to far and I'm not convinced whether the flexibility outweighs the added complexity.

  // Library implementation A print a warning and is happy to have the parser
  // continue after the warning.
  Error parseCallback(Error ParseError)
  {
    if (ParseError) {
      WithColor(errs(), HighlightColor::Warning).get() << "warning: ";
      errs() << Message;
    }
    return Error::succes();
  }
  
  // Library implementation B is more pedantic and wants parsing to abort after a
  // warning.
  Error parseCallback(Error ParseError)
  {
    return ParseError;
  }
  
  Error DWARFDebugLine::LineTable::parse(...)
  {
    ... if (!State.Sequence.Empty)
    {
      // The parser checks whether the 'minor issue' has been dealt with by the
      // callback. If not, it aborts parsing and returns the error.
      if (Error Err = parseCallback(createError("last sequence in debug line table is not terminated!")))
        return Err;
    }
    ...
  }

If we go this route we could have the same interface for `handleDebugLineParseErrors` and communicate that we aborted parsing because of a fatal error.

  Error llvm::handleDebugLineParseErrors(Error ParseErrors) {
    bool FatalError = false;
    handleAllErrors(std::move(ParseErrors), ...);
    if (FatalError) 
      return make_error<StringError>("Parsing of this line table was aborted because fatal error encountered");
    return Error:succes();
  }



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:36
+  // section.
+  DebugLineError(bool IsFatal, const Twine &Msg)
+      : IsFatal(IsFatal), Msg(Msg.str()) {}
----------------
jhenderson wrote:
> JDevlieghere wrote:
> > How about switching the order and giving the bool a default value (`false`?)
> I tried doing this, but there was little benefit, since this is always created via createError, which is a function with variadic templates, making it impossible (to my knowledge) to use a default argument in createError. I could still change it, but `IsFatal` will still be specified explicitly every time it is called.
Alright, I'll leave the decision up to you. My reasoning was that it doesn't harm and might make things easier in the future if we ever want to construct this error without the createError helper. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:65
+/// \param Message The message to report.
+void warnForMinorIssue(StringRef Message);
+
----------------
jhenderson wrote:
> JDevlieghere wrote:
> > I think something like `warn` would be sufficiently clear.
> It's a namespacing thing - I don't want this function confused with, say, LLD's "warn" function, which may not do the same thing. I guess an argument could be made for adding a "warn" function the Support library, or similar, though I'm not sure about that. Maybe I could make it a static method of DWARFDebugLine?
Makes sense, the static method sounds good to me. 


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:42
+                  [&](DebugLineError &Info) {
+                    WithColor(errs(), HighlightColor::Warning).get()
+                        << "warning: ";
----------------
Should a fatal error not be displayed as an error instead of a warning?


Repository:
  rL LLVM

https://reviews.llvm.org/D44560