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

[Tobias Hieta via Phabricator via llvm-commits]

thieta 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
----------------
jhenderson wrote:
> 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`?
Ideally we should change the api to not return a int status from the demangling function instead using DemangleError() - as I noted in my other comment, I was trying to preserve the old API because I thought it was wide-spread. But that doesn't seem to be the case, so I might end up just fixing that instead of making this  a normal int.


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:53
+    case llvm::DemangleError::Success:
+      return "success.";
+    }
----------------
jhenderson wrote:
> 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.
Hmm seems like it should apply here, I will make it consistent.


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:66
+
+std::optional<const char *> itaniumDemangleWithError(const char *M,
+                                                     std::error_code &E);
----------------
jhenderson wrote:
> 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.
Agreed.


================
Comment at: llvm/lib/Demangle/Demangle.cpp:75-78
+  if (!isItaniumEncoding(M)) {
+    E = llvm::make_error_code(DemangleError::InvalidFormat);
+    return {};
+  }
----------------
jhenderson wrote:
> 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.
We can do that - but it becomes a little more complex and we probably need to audit all the places where the demangling functions are used now in that case.

`itaninumDemangle()` calls: `AbstractManglingParser::parse()` which returns `nullptr` if there is any errors, but have no status reports here.

So we could either:
1) change the `AbstractManglingParser` API to return the `error_code` from there instead. This seems possible since it's only used internally in the itaniumDemangler - it's not exposed from there (wonder why the implementation lives in a header in that case). The function also seems to be aware when it's not starting with __Z (or rather it tries to find *Z and we can probably return InvalidFormat if it doesn't). We could change this and either keep the current itaniumDemangle() function which will change the error_code to the current status int.
2) change itaniumDemangle() to do something similar to what isItaniumEncoding does. but this seems to just move the responsibility for it.

I was going to say that it's harder to change the API of `itaniumDemangle` since it's used "everywhere" - but that doesn't seem to be true - searching for it only seems to happen in 10 places:

https://github.com/search?q=repo%3Allvm%2Fllvm-project+itaniumDemangle%28+language%3AC%2B%2B&type=code&l=C%2B%2B

I am going to give it a try to move the logic to AbstractManglingParser to return a error_code instead and see if it can work.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139298