[PATCH] D139298: WIP: Add error handling to demangle API

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:27
+
+enum class DemangleError {
+  InvalidFormat = -5, // When a specific demangler can't handle this string
----------------
I generally support the use of `enum class`, but the enums are generally being cast to an `int`, so maybe that's a sign that this should just be a plain `enum`?


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:28
+enum class DemangleError {
+  InvalidFormat = -5, // When a specific demangler can't handle this string
+  Unknown = -4,
----------------
Nit: comments should end with "." (here and below).


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:53
+    case llvm::DemangleError::Success:
+      return "success.";
+    }
----------------
Nit: capitalization here is inconsistent.

FWIW, the LLVM coding standards state for warning and error messages to a) start with a lower case letter, and b) not to end with a period. Whether that applies in this context is debatable.


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:66
+
+std::optional<const char *> itaniumDemangleWithError(const char *M,
+                                                     std::error_code &E);
----------------
I'd just overload the function name, rather than appending "WithError". The fact that it takes a `std::error_code` reference indicates that it will do something to that field, and it's then similar to e.g. how std::filesystem overloads function.


================
Comment at: llvm/lib/Demangle/Demangle.cpp:75-78
+  if (!isItaniumEncoding(M)) {
+    E = llvm::make_error_code(DemangleError::InvalidFormat);
+    return {};
+  }
----------------
dblaikie wrote:
> Part of what I was hoping for was that we wouldn't need an explicit `isItaniumEncoding` check, even inside the implementation - that `Demangler::parse` could produce that as a separate error result if it parsed something without the desired prefix. Would that be possible?
Not sure if this is what @dblaikie is suggesting, but it seems to me that you could have the underlying demangling code set a status of "NotThisFormat" or similar if it encounters something with the wrong prefix type. This would enable calling code to know whether to try the next format, or just stop. That might not work so well in the face of type demangling though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139298/new/

https://reviews.llvm.org/D139298