[PATCH] D111417: [Demangle] Add support for D symbols back referencing

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Looks like this is a bit undertested - the number encoding supports uppercase values, but I don't think the test case tests any uppercase (multi-digit/character encoded) values, for instance? As well as various (I count about 3-4 or so, at least) different encoding failure cases that should probably be tested too.



================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:136-147
+  /// Extract the back reference position from a given string
+  ///
+  /// \param Mangled string to extract the back reference position
+  /// \param Ret assigned result value
+  ///
+  /// \return the remaining string on success or nullptr on failure
+  ///
----------------
Perhaps it'd make sense if this took Mangled as a const char** in/out, and returned Optional<Ret>? (both callers overwrite their Mangled parameter with the return result anyway - so seems simpler for it to be in/out?) if Optional isn't available, maybe long min, -1, something could be the invalid result value?

(similarly for decodeBackref)


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:386-387
+      Val += Mangled[0] - 'a';
+      if ((long)Val <= 0)
+        break;
+      *Ret = Val;
----------------
is this an overflow check? Can it be reached if the other overflow check at 379 succeeds? (is it tested?)


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:402-403
+
+  if (Mangled == nullptr || *Mangled != 'Q')
+    return nullptr;
+
----------------
Looks like this condition's probably untested? Should it be removed or turned into an assertion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111417