Page MenuHomePhabricator

[Demangle][Rust] Print special namespaces
ClosedPublic

Authored by tmiasko on May 4 2021, 2:02 AM.

Diff Detail

Event Timeline

tmiasko created this revision.May 4 2021, 2:02 AM
tmiasko requested review of this revision.May 4 2021, 2:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 2:02 AM

Any chance this could be subdivided further to implement only one grammar rule or the like? It'd make it much easier to inspect the test quality with smaller increments like that.

llvm/include/llvm/Demangle/RustDemangle.h
62–63

We tend not to use 'const' on locals, and maybe especially not on parameters (since it's even more readily confused with const-ref on parameters).

llvm/lib/Demangle/RustDemangle.cpp
209–250

This whole thing is alphabetic except for 'p' - maybe put it in order?

Also - maybe use a const char* array indexed on C - 'a'? Not necessary by any means, though.

269–298

Perhaps the recursion system could be a scoped device, making it easier to add/less error-prone in case of early returns, etc?

Recurse R(Error, RecursionLimit); // or just take *this and befriend for access to Error and RecursionLimit
if (!R)
  return;
// Recurse's dtor decrements the RecursionLimit)
tmiasko updated this revision to Diff 343423.May 6 2021, 8:42 AM
tmiasko marked 2 inline comments as done.
  • Limit changes to M / X / Y path productions
  • Use SwapAndRestore for recursion limit, printing mode, and parser position.
tmiasko updated this revision to Diff 343425.May 6 2021, 8:43 AM
tmiasko retitled this revision from Demangle Rust paths, basic types and named types to [Demangle][Rust] Parse paths and named types.

Update description

Could this be cut down further to be paths OR named types, rather than both? (I know this seems fussy/like a pain & I'm sorry about that - but if it gets down to the smallest thing possible (like adding only one named type and associated tests) I'm hoping it'll be super easy to review and verify the tests/understand how the grammar is implemented, etc - as it stands currently it does take me a while to understand each test case, which set of grammar rules are being tested relative to which different rules are being implemented in the patch, etc)

tmiasko updated this revision to Diff 343762.May 7 2021, 2:31 PM
tmiasko retitled this revision from [Demangle][Rust] Parse paths and named types to [Demangle][Rust] Print special namespaces.

Print special namespaces

Could this be cut down further to be paths OR named types, rather than both?

I reduced the changes further still. To introduce further path productions I need types, and the one already implemented type is a named type, i.e., a path. Grammar limits how much I can break it down, but I will see what I can do.

dblaikie added inline comments.May 7 2021, 5:18 PM
llvm/test/Demangle/rust.test
14–25

Do these benefit from being inside two namespaces? I think it'd be great if features were tested as much in isolation as possible, with maybe a single test demonstrating that a feature works in a particular context (which is really testing the outer production - and should used the simplest instance of the inner production as possible).

There's also a rather large and, so far as I can tell, arbitrary, disambiguator ('s21hi0yVfW1J') - is there a particular reason for that being present, and having such a long/complicated value?

ie, maybe these tests could be:

_RNCC5crate0
_RNCC5crates_0
_RNCNCC5crates_00
_RNCNCC5crates_01g

& I'm still thinking maybe some kind of comment would be useful to describe these things, though I'm not sure exactly what.

_R N [ C [ C 5crate ] 0 ]
_R N [ C [ C 5crate ] [ s _0 ] ]
_R N [ C [ N [ C [ C 5crate ] [ s _0 ] ] ] 0 ]

In any case - could you simplify down the test cases to make them as narrow and legible/obvious as possible? Maybe add some comments about the invalid/mangled cases - where the invalidity is interesting, for instance? But hopefully if they're simplified way down, the invalidity will be more clear/obvious?

tmiasko updated this revision to Diff 343817.May 8 2021, 12:22 AM

Simplify test cases

llvm/test/Demangle/rust.test
14–25

I used rustc to generate the test cases previously. The extra nesting is necessary at language level. The long disambiguator s21hi0yVfW1J is a crate hash. Together with a crate name it forms a unique global identifier of the crate (c++filt shows / omits those hashes output based on -i flag, and I would like to implement something similar later on).

Neither of those aspect is strictly necessary for testing, so I reduced the test cases, but they no longer correspond to valid Rust programs.

dblaikie accepted this revision.May 8 2021, 7:52 PM

Looks great - thanks for sticking with me/helping make this easier for me to follow.

llvm/test/Demangle/rust.test
33–34

Might be worth consistently using certain easy to identify identifiers? (I at least find it easier to understand the tests when the identifiers are readily.. identifiable - 'crate' is easy to spot in the other examples, for instance - but in an example with single letter identifiers like this ('a' and 'c') can make it a bit harder to read the example - knowing where the user-provided names start/end, which parts aren't from the user identifier)

(similarly below in the invalid namespace)

I guess in the first few examples the manglings were short/simple enough for me to figure out which component was from what - but as the examples get more complicated it's a bit harder to follow/understand and maybe clearer/long/obvious names would make it a bit easier.

This revision is now accepted and ready to land.May 8 2021, 7:52 PM
tmiasko updated this revision to Diff 343908.May 9 2021, 4:16 AM

Use easy to distinguish identifiers in test cases

Could you commit this for me? Thanks.

This revision was landed with ongoing or failed builds.May 9 2021, 3:46 PM
This revision was automatically updated to reflect the committed changes.