[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:418-420
+  APInt Value(HexDigits.size() * 4 /* NumBits */,
+              StringRef(HexDigits.begin(), HexDigits.size()), 16 /* Radix */);
+  SmallString<40> Str;
----------------
Hmm - I'm just realizing that it looks like the demangle library has no dependencies on the rest of LLVM (the StringView type was a hint I saw earlier - but now looking more closely).

I'm guessing that's intentional & I'm surprised this code links successfully - I'd expect some part of SmallString or APInt would have some link-time dependence on the rest of LLVM, which I think (looking at the CMake file at least) there's no dependency on from the demangle library? Hmm, maybe llvm-cxxfilt has such a dependency and it's being picked up by accident (are you using lld or a windows linker, by chance? Those will be a bit more flexible about fulfilling library dependencies, perhaps).

So maybe we'll have to stick with the previous implementation you had here :/ (if so, might be worth a comment explaining why - though I don't immediately know where this constraint for libDemangle is documented/comes from - could you check around a bit in the library/version control history, maybe?)


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:288
 
+// <const> = <basic-type> <const-data>
+//         | "p"                          // placeholder
----------------
tmiasko wrote:
> 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?
Looks pretty good - if you like, you could commit that as a preliminary refactoring-only commit. (might be a good point to follow the steps to get commit access for changes like this)


================
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>
----------------
tmiasko wrote:
> 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).
perhaps rather than relying on the type encoding to test the integer encoding, the type could be rendered in the name (replacing the "integer" string) instead? (though that may be harder to read in the mangling - maybe not though)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102179