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

[Tomasz Miąsko via Phabricator via llvm-commits]

tmiasko 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'))
----------------
dblaikie wrote:
> 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)
No particular reason. I reverted the changes so that code falls back to the type production as before.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:288
 
+// <const> = <basic-type> <const-data>
+//         | "p"                          // placeholder
----------------
dblaikie wrote:
> 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?
I introduced an enum for a basic types and reused the parsing code. What do you think?


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:327-332
+  if (HexDigits.size() <= 16) {
+    printDecimalNumber(Value);
+  } else {
+    print("0x");
+    print(HexDigits);
+  }
----------------
dblaikie wrote:
> 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.
I used APInt to consistently format values as decimals. This matches rustc pretty printing which consistently uses decimal as well. 

In itanium this is non-issue since values have decimal mangling.


================
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>
----------------
dblaikie wrote:
> 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?
I included a type argument in addition to a const integer value,  so that demangled from clearly indicates what type is being tested, and to visually distinguish checks from each other (in case one of them fails for example).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102179