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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Hi James, thanks for taking the time to come up with an alternative. I very much prefer this approach over the previous differential!



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:34
+
+  // A fatal DebugLineError is one which prevents further reading of the
+  // section.
----------------
Let's move this comment to isFatal. I think the name of the variable is sufficiently self-explanatory for the constructor. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:36
+  // section.
+  DebugLineError(bool IsFatal, const Twine &Msg)
+      : IsFatal(IsFatal), Msg(Msg.str()) {}
----------------
How about switching the order and giving the bool a default value (`false`?)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:47
+private:
+  bool IsFatal;
+  std::string Msg;
----------------
It won't matter here (yet) but padding-wise it's (generally) better to have the smaller types last. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:59
+/// in the section.
+bool handleDebugLineParseErrors(Error ParseErrors);
+
----------------
How do you feel about the following idea: what if we use this function as the callback and have it take *and* return an error. If the function handles the error (i.e. print it), this returns success, and the caller continues. If the error is fatal and/or prevents further parsing, we defer the problem and we have the parse method return the underlying error. Would that fit this use case?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:65
+/// \param Message The message to report.
+void warnForMinorIssue(StringRef Message);
+
----------------
I think something like `warn` would be sufficiently clear.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:286
+          const DWARFContext &Ctx, const DWARFUnit *U,
+          std::function<void(StringRef)> MinorIssueCallback = warnForMinorIssue,
+          raw_ostream *OS = nullptr);
----------------
I'm not convinced on the minor/major nomenclature. I think we could two things here:

 -  Rename this to something like `warnCallback` to make it clear that this is what the callback is for.
 - Make this a callback that takes an error *and* returns an error, allowing the callback to decide whether to handle it and how. Probably I'm over-engineering it here, but conceptually it would be nice to have the client decide whether the issue is "minor" or not. For example, one client could decide to just print the input error as a warning (and then return success(), while another could simply forward the error. This does add complexity to the parse method, as we then would need to check whether the callback returned success or not, and return the error in the latter case. 

FWIW I don't think we need the second option here, but from an interface-design point of view it could be interesting. 


Repository:
  rL LLVM

https://reviews.llvm.org/D44560