This is an archive of the discontinued LLVM Phabricator instance.

ASTImporter: improve support for C++ templates
ClosedPublic

Authored by a.sidorin on Nov 16 2016, 10:23 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

a.sidorin updated this revision to Diff 78212.Nov 16 2016, 10:23 AM
a.sidorin retitled this revision from to ASTImporter: improve support for C++ templates.
a.sidorin updated this object.
a.sidorin added reviewers: spyffe, khazem.
a.sidorin added a subscriber: cfe-commits.
khazem edited edge metadata.Nov 16 2016, 10:49 AM

Thanks very much for this patch! It certainly fixes the infinite recursion issue on our codebase. It LGTM, but I'd like to add a test case before landing it.

spyffe requested changes to this revision.Nov 16 2016, 11:15 AM
spyffe edited edge metadata.

This looks amazing. I have a few minor quibbles and a testing concern, but overall this looks like a great step forward! Thank you!

lib/AST/ASTImporter.cpp
458 ↗(On Diff #78212)

Is this really an appropriate default result? I would argue for false here so that an error would propagate, as is typical in ASTImporter.
Note that this does disagree with the original source's true but I think that was because we knew we didn't handle anything, whereas now the assumption is we handle everything.

496 ↗(On Diff #78212)

We should probably also check whether DN1->isIdentifier() == DN2->isIdentifier().

520 ↗(On Diff #78212)

As above, I'd argue for false here now that we're flipping to the assumption that this code is complete.

4911 ↗(On Diff #78212)

Could you assign this to a variable here, to avoid the redundant call a line below?

4931 ↗(On Diff #78212)

This blank line is probably not needed.

7035 ↗(On Diff #78212)

Is this function properly covered by the test? I would like to see some deeply-neded name specifiers in the test, with entries for all the cases here.
If I'm missing the part of the test that covers this, please let me know.

This revision now requires changes to proceed.Nov 16 2016, 11:15 AM
a.sidorin updated this revision to Diff 78382.Nov 17 2016, 11:40 AM
a.sidorin edited edge metadata.

Address review comments; fix tests.

Thank you! I'll update this review again when I have a test for NestedNameSpecifierLocs.

lib/AST/ASTImporter.cpp
458 ↗(On Diff #78212)

Good point. Default false will also fail import if a new non-handled property or option will be added in AST and clearly indicate an error so I will change it.

a.sidorin updated this revision to Diff 79054.Nov 23 2016, 4:52 AM
a.sidorin edited edge metadata.

Add a simple test for import of complex NestedNameSpecifierLocs.

spyffe accepted this revision.Nov 28 2016, 1:57 PM
spyffe edited edge metadata.

Yeah, that test looks great! Thanks!

lib/AST/ASTImporter.cpp
496 ↗(On Diff #78212)

Looking at my comment with fresh post Thanksgiving eyes, that would be totally wrong. The IsStructurallyEquivalent is fine.

test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec2.cpp
67 ↗(On Diff #79054)

ooh, nice!

This revision is now accepted and ready to land.Nov 28 2016, 1:57 PM

Sorry for the late comment, but one of the tests that this introduces is breaking on my end:

/usr/local/google/home/khazem/doc/llvm/tools/clang/test/ASTMerge/class-template-partial-spec.cpp:9:11: error: expected string not found in input
// CHECK: /media/build/smrc-llvm/master/llvm/tools/clang/test/ASTMerge/Inputs/class-template-partial-spec2.cpp:21:32: error: external variable 'X1' declared with incompatible types in different translation units ('TwoOptionTemplate<int, double>' vs. 'TwoOptionTemplate<int, float>')
          ^
<stdin>:1:1: note: scanning from here
/usr/local/google/home/khazem/doc/llvm/tools/clang/test/ASTMerge/Inputs/class-template-partial-spec1.cpp:20:30: error: external variable 'X0' defined in multiple translation units
^
<stdin>:7:15: note: possible intended match here
/usr/local/google/home/khazem/doc/llvm/tools/clang/test/ASTMerge/Inputs/class-template-partial-spec2.cpp:21:32: error: external variable 'X1' declared with incompatible types in different translation units ('TwoOptionTemplate<int, double>' vs. 'TwoOptionTemplate<int, float>')
              ^

Is this expected?

I didn't notice this failure but I'll recheck this. Thank you Kareem!

Kareem, I have re-checked it and I cannot see the failure. But I'm not going to commit it until NY holidays end (and, anyway, I will not commit it if somebody has failing tests). Could you describe your configuration?

I got it. I have hard-coded paths in CHECK-lines so these tests are passed on my machine but not on other. Thank you Kareem!

This revision was automatically updated to reflect the committed changes.

Main revisions: rL292776, rL292778. Sorry for not mentioning them in Differential Revision.