This is an archive of the discontinued LLVM Phabricator instance.

Differential D38643

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

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

Details

Reviewers
arphaman
Commits
rG663ba15ed6cf: PR13575: Fix USR mangling for fixed-size arrays
rC315236: PR13575: Fix USR mangling for fixed-size arrays
rL315236: PR13575: Fix USR mangling for fixed-size arrays
Summary

Added array type mangling to USR generation. Included test from bug report.

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.

jkorous-apple updated this revision to Diff 118066.Oct 6 2017, 1:35 PM

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'.

jkorous-apple updated this revision to Diff 118104.Oct 6 2017, 4:05 PM

Ammenended as suggested.

arphaman added a comment.Oct 6 2017, 4:21 PM

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.

jkorous-apple added a comment.Oct 6 2017, 5:04 PM

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?

jkorous-apple updated this revision to Diff 118115.Oct 6 2017, 5:47 PM

Solved issues from CR.

jkorous-apple updated this revision to Diff 118231.Oct 9 2017, 10:16 AM

clang-format

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.

jkorous-apple updated this revision to Diff 118247.Oct 9 2017, 11:51 AM

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

Closed by commit rL315236: PR13575: Fix USR mangling for fixed-size arrays (authored by jkorous). · Explain WhyOct 9 2017, 12:53 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
cfe/
trunk/
lib/
Index/
19 lines
test/
Index/
USR/
11 lines

Diff 118257

cfe/trunk/lib/Index/USRGeneration.cpp

Show First 20 Lines • Show All 810 Lines • ▼ Show 20 Lines if (const InjectedClassNameType *InjT = T->getAs<InjectedClassNameType>()) {
continue; continue;
} }
if (const auto *VT = T->getAs<VectorType>()) { if (const auto *VT = T->getAs<VectorType>()) {
Out << (T->isExtVectorType() ? ']' : '['); Out << (T->isExtVectorType() ? ']' : '[');
Out << VT->getNumElements(); Out << VT->getNumElements();
T = VT->getElementType(); T = VT->getElementType();
continue; continue;
} }
if (const auto *const AT = dyn_cast<ArrayType>(T)) {
Out << '{';
switch (AT->getSizeModifier()) {
case ArrayType::Static:
Out << 's';
break;
case ArrayType::Star:
Out << '*';
break;
case ArrayType::Normal:
Out << 'n';
break;
}
if (const auto *const CAT = dyn_cast<ConstantArrayType>(T))
Out << CAT->getSize();
T = AT->getElementType();
continue;
}
// Unhandled type. // Unhandled type.
Out << ' '; Out << ' ';
break; break;
} while (true); } while (true);
} }
void USRGenerator::VisitTemplateParameterList( void USRGenerator::VisitTemplateParameterList(
▲ Show 20 Lines • Show All 219 Lines • Show Last 20 Lines

cfe/trunk/test/Index/USR/array-type.cpp

// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
// Function template specializations differing in array type parameter should have unique USRs.
template<class buffer> void foo(buffer);
// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n16C>#*C# | __Z3fooIA16_cEvT_ | Decl,RelSpecialization | rel: 1
template<> void foo<char[16]>(char[16]);
// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n32C>#*C# | __Z3fooIA32_cEvT_ | Decl,RelSpecialization | rel: 1
template<> void foo<char[32]>(char[32]);
// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n64C>#*C# | __Z3fooIA64_cEvT_ | Decl,RelSpecialization | rel: 1
template<> void foo<char[64]>(char[64]);