[PATCH] D102179: [Demangle][Rust] Parse integer constants

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:203-204
 void Demangler::demangleGenericArg() {
-  // FIXME parse remaining productions
-  demangleType();
+  if (consumeIf('L'))
+    Error = true; // FIXME demangle lifetimes.
+  else if (consumeIf('K'))
----------------
Previous code didn't have an error path for this missing piece - any particular reason to add it now (& thus have to test it) compared to not supporting this & having const or type as the only options (& if it's not const, parse it as a type - as the previous code did, parsing everything as a type regardless)


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:288
 
+// <const> = <basic-type> <const-data>
+//         | "p"                          // placeholder
----------------
Somewhat surprising that though this production uses `<basic-type>` but the implementation doesn't call `parseBasicType` - any chance of sharing that implementation detail, trivial as it is?


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:319
+
+void Demangler::demangleConstInt() {
+  bool IsNegative = consumeIf('n');
----------------
grammar production comment for this?


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:327-332
+  if (HexDigits.size() <= 16) {
+    printDecimalNumber(Value);
+  } else {
+    print("0x");
+    print(HexDigits);
+  }
----------------
switching between int and decimal for the demangling seems like it could be problematic for readers/parsers/etc, maybe? (what does llvm-cxxfilt do for equivalent C++ numbered values (like integer template parameters))

Maybe APSInt could be used which supports bigger (though not arbitrarily big, I don't think... - I think you have to pick the size, though I guess you could pick it dynamically based on the number of hex digits) integers.


================
Comment at: llvm/test/Demangle/rust.test:151-185
+CHECK: integer::<0, i8>
+       _RIC7integerKa0_aE
+
+CHECK: integer::<0, u8>
+       _RIC7integerKh0_hE
+
+CHECK: integer::<0, isize>
----------------
Do these benefit from the second parameter that shows the type? Or is the type testing redundant with the existing type testing added in the previous patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102179