This is an archive of the discontinued LLVM Phabricator instance.

[Demangle][Rust] Parse dyn-bounds
ClosedPublic

Authored by tmiasko on May 26 2021, 3:49 AM.

Details

Diff Detail

Event Timeline

tmiasko created this revision.May 26 2021, 3:49 AM
tmiasko requested review of this revision.May 26 2021, 3:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 3:49 AM

Could you split this up - one patch to add dyn-bounds with zero dyn-traits, then add dyn-traits with zero dyn-trait-assoc-binding, then add those? (takes me a while to check the test cases/understand exactly what they're doing - so having really narrow patches makes it easier to focus on the difference between one patch and the next)

tmiasko updated this revision to Diff 348342.May 27 2021, 11:42 AM
tmiasko retitled this revision from [Demangle][Rust] Parse trait objects to [Demangle][Rust] Parse dyn-bounds.

Parse dyn-bounds without any dyn-traits

At language level at least one trait is required, so visually this doesn't look
quite right without any traits and trailing whitespace at the end.

dblaikie added inline comments.Jun 1 2021, 4:12 PM
llvm/lib/Demangle/RustDemangle.cpp
485–492

keep thinking it'd be nice if this were wrapped up in some (albeit parameterized for different situations you mentioned) "demangleLifetime" to match the syntax description more closely

554–555

According to the grammar/spec this is <binder> = "G" <base-62-number> rather than <binder> = ["G" <base-62-number>]?

555–556

Not sure how consistently this is done throughout the code - but maybe including "optional" when a feature is optional (so it's clear it's implementing [<binder>] not <binder>) would be good?

llvm/test/Demangle/rust.test
243–244

Could this test be simplified down to:

CHECK: trait::<dyn for<'b> + 'a>
       _RIC5traitDG_EL_E
tmiasko updated this revision to Diff 349188.Jun 2 2021, 1:04 AM
tmiasko marked 2 inline comments as done.

Rebase

llvm/lib/Demangle/RustDemangle.cpp
485–492

I am still somewhat reluctant to change this, since I would have to introduce three different variants and each would be used only once. For example, in variant here, when lifetime is printed it should be preceded by + .

555–556

I renamed the function in parent revision to demangleOptionalBinder.

llvm/test/Demangle/rust.test
243–244

In "D" <dyn-bounds> <lifetime> production, <lifetime> is outside the binder from <dyn-bounds>. Additionally since erased lifetimes are not printed here, test has to use another binder.

dblaikie accepted this revision.Jun 3 2021, 8:03 PM

Looks good - thanks for walking me through it!

This revision is now accepted and ready to land.Jun 3 2021, 8:03 PM
This revision was landed with ongoing or failed builds.Jun 7 2021, 9:25 AM
This revision was automatically updated to reflect the committed changes.