This is an archive of the discontinued LLVM Phabricator instance.

PR13575: Fix USR mangling for fixed-size arrays.
ClosedPublic

Authored by jkorous-apple on Oct 6 2017, 1:10 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jkorous-apple created this revision.Oct 6 2017, 1:10 PM
arphaman edited edge metadata.Oct 6 2017, 1:28 PM

Thanks for working on this!
Could you please post the patch with full context (git diff -U999999)?

test/Index/USR/array-type.cpp
1 ↗(On Diff #118060)

Please use FileCheck and verify the exact USR strings. This way you'll know that they're unique without actually trying to verify if they're unique in the test.

Uploaded full diff.

arphaman added inline comments.Oct 6 2017, 1:50 PM
lib/Index/USRGeneration.cpp
820 ↗(On Diff #118066)

We should probably follow the other types and just set T = AT->getElementType() instead of using VisitType and continue instead of returning at the end of this if.

821 ↗(On Diff #118066)

The "[" "]" syntax could collide with the vector-type USRs. What about using another character? Maybe '{'?

826 ↗(On Diff #118066)

I think it's better to check 'ArrayType::Normal' instead of default to ensure we will be able to distinguish between added size modifiers that could be added in the future. We should also probably give it some representation, like 'n'.

Ammenended as suggested.

Thanks, it looks much better! A couple more comments:

lib/Index/USRGeneration.cpp
819 ↗(On Diff #118104)

You can use const auto * here.

826 ↗(On Diff #118104)

Ditto. Also, the braces are redundant.

829 ↗(On Diff #118104)

I don't think we need the terminating character.

Replied and going to delete the end delimiter.

lib/Index/USRGeneration.cpp
819 ↗(On Diff #118104)

That's what I wanted to do in the first place but decided against it because of consistency - see other if conditions around. Do you still advice to use auto?

826 ↗(On Diff #118104)

Braces might be redundant yet I use them intentionally even for such cases (think the infamous goto bug). But I can definitely delete those if you insist. BTW Is there some consensus about this?

829 ↗(On Diff #118104)

I agree, going to delete that.

test/Index/USR/array-type.cpp
1 ↗(On Diff #118060)

Since the USR format doesn't seem to be really stable I wanted to avoid hard-coding USR values in tests. Wouldn't those tests be unnecessary brittle in the sense that hard-coded values would have to be modified in case of future changes?

Solved issues from CR.

arphaman accepted this revision.Oct 9 2017, 10:26 AM

LGTM

lib/Index/USRGeneration.cpp
819 ↗(On Diff #118231)

Nit: I don't think you really need the 2nd const here and in the next if.

This revision is now accepted and ready to land.Oct 9 2017, 10:26 AM
arphaman added inline comments.Oct 9 2017, 10:27 AM
lib/Index/USRGeneration.cpp
820 ↗(On Diff #118231)

You might also want to use the character literals for one char strings for efficiency.

Single char constants don't need to be c-strings.

This revision was automatically updated to reflect the committed changes.