This is an archive of the discontinued LLVM Phabricator instance.

[Demangle][Rust] Parse tuples
ClosedPublic

Authored by tmiasko on May 16 2021, 7:03 AM.

Details

Diff Detail

Event Timeline

tmiasko created this revision.May 16 2021, 7:03 AM
tmiasko requested review of this revision.May 16 2021, 7:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2021, 7:04 AM
dblaikie added inline comments.May 16 2021, 10:26 AM
llvm/lib/Demangle/RustDemangle.cpp
452–453

Looks a bit awkward to print a comma after a single argument ((_,)) - is that necessary? (does it disambiguate this with some other case that has only a list of 1 between parentheses?)

tmiasko added inline comments.May 16 2021, 10:48 AM
llvm/lib/Demangle/RustDemangle.cpp
452–453

The intention is to match Rust language syntax which requires a comma after one element tuples to disambiguate them from parenthesized types.

dblaikie accepted this revision.May 16 2021, 10:55 AM
dblaikie added inline comments.
llvm/lib/Demangle/RustDemangle.cpp
452–453

Cool - thanks for the context!

And stylistically, that trailing comma is usually written without a space after it, unlike separating commas?

Pity, this could come out more naturally from the algorithm if that was the case (always printing ", " after every type - rather than having to special case)

I guess you could do it this way:

for (...) {
  if (I > 0)
    print(" ");
  demangleType();
  print(",");
}

Marginally less special casing

This revision is now accepted and ready to land.May 16 2021, 10:55 AM
tmiasko updated this revision to Diff 345884.May 17 2021, 8:01 AM
tmiasko marked 2 inline comments as done.

Rebase

llvm/lib/Demangle/RustDemangle.cpp
452–453

Looking at rustfmt, rustc type printer and other demanglers, they are consistent writing trailing comma without a space. They also include the trailing comma only when necessary, i.e., for one element tuple only (so I retained the special case).

This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.May 19 2021, 11:48 AM
llvm/lib/Demangle/RustDemangle.cpp
452–453

Cool, I still wonder if it might be marginally more neatly written the way I suggested - it still produces the same output, but with one fewer "if", I think?

tmiasko added inline comments.May 19 2021, 11:58 AM
llvm/lib/Demangle/RustDemangle.cpp
452–453

It would be (), (T1,), and then (T1, T2,) instead of (T1, T2).

dblaikie added inline comments.May 19 2021, 12:08 PM
llvm/lib/Demangle/RustDemangle.cpp
452–453

Oh, right. Thanks for setting me straight!