Page MenuHomePhabricator

[Demangle] Add support for multiple identifiers in D qualified names
AcceptedPublic

Authored by ljmf00 on Fri, Nov 19, 7:10 PM.

Details

Reviewers
dblaikie

Diff Detail

Event Timeline

ljmf00 created this revision.Fri, Nov 19, 7:10 PM
ljmf00 requested review of this revision.Fri, Nov 19, 7:10 PM
dblaikie accepted this revision.Sat, Nov 20, 1:16 PM

Looks good, thanks!

llvm/lib/Demangle/DLangDemangle.cpp
200–210

Might be more legible to write this as:

bool First = true;
do {
  if (First)
    *Demangled << '.';
  First = false;
  Mangled = parseIdentifier...

Rather than incrementing N which is rather non-descript.

This revision is now accepted and ready to land.Sat, Nov 20, 1:16 PM
ljmf00 added inline comments.Sat, Nov 20, 1:38 PM
llvm/lib/Demangle/DLangDemangle.cpp
200–210

Yeah, that's is better and more performant, although I think you meant the other way around although. The intention here is to add dot on every iteration other than the first.

dblaikie added inline comments.Sun, Nov 21, 6:20 PM
llvm/lib/Demangle/DLangDemangle.cpp
200–210

Right right!

ljmf00 updated this revision to Diff 389873.Thu, Nov 25, 1:14 PM

Done @dblaikie, can you review?

dblaikie accepted this revision.Thu, Nov 25, 6:55 PM

Done @dblaikie, can you review?

Oh, generally if something's "Approved with suggestions" it's fine to submit it if you think you've applied the suggestions. So, this is fine, but I'll mark it as approved again.

llvm/lib/Demangle/DLangDemangle.cpp
201–205

I'd probably make this "First" rather than "NotFirst" - might be a bit clearer/more direct.

ljmf00 added a comment.EditedFri, Nov 26, 7:05 AM

Done @dblaikie, can you review?

Oh, generally if something's "Approved with suggestions" it's fine to submit it if you think you've applied the suggestions. So, this is fine, but I'll mark it as approved again.

Yeah, I don't have permission to land it, but thanks for letting me know that. A bit offtopic, but what are the rules to apply for commit access? I believe it is early to request that, although.

EDIT: It is not landable yet, as it has dependencies not yet approved

Done @dblaikie, can you review?

Oh, generally if something's "Approved with suggestions" it's fine to submit it if you think you've applied the suggestions. So, this is fine, but I'll mark it as approved again.

Yeah, I don't have permission to land it, but thanks for letting me know that. A bit offtopic, but what are the rules to apply for commit access? I believe it is early to request that, although.

Generally once you've had a few patches approved, you can request pre-review commit access ( https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access - though the documentation might be a bit out of date, I'd probably still email Chris and he can bounce it to whomever & maybe update the docs) & then you sort of play it by ear - anything that you have a pretty good estimate will be trivially approved, you can eventually commit without pre-commit review (& be responsive to post-commit review).

EDIT: It is not landable yet, as it has dependencies not yet approved

Ah, cool - just ping here if you need me to commit it when it comes time.