This is an archive of the discontinued LLVM Phabricator instance.

[clang][ASTImporter] Import TemplateName correctly
ClosedPublic

Authored by balazske on Feb 23 2023, 12:28 AM.

Details

Summary

This is a fix for a problem when multiple template
specializations are created by ASTImporter for the
same specialization. The problem happens if a
TemplateName is imported that points to a template
delcaration (for a template template argument)
(specialization) that has multiple instances in the
declaration chain. If two TemplateName objects contain
different pointers to a template specialization,
these TemplateName objects will have different checksum
even if they point into the same declaration chain.
The problem is fixed if the canonical declaration is used.

Diff Detail

Event Timeline

balazske created this revision.Feb 23 2023, 12:28 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
balazske requested review of this revision.Feb 23 2023, 12:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 12:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This patch needs a unit test (as @balazske mentioned). So far, the case we have is too large to be a suitable unittest or lit case - so requires reduction. @balazske , will you be adding this as a unittest or lit case?

Also, I think this patch needs to be integrated with D144273 IMHO, since they address similar issues.

I verified the patch stack D144273, D140562, and D144622 (this one) addresses the problems we've seen as a result of D133468. When the patches are ready, a brief history should be included in the commit header.

I think these patches are fix for separate problems and can be applied independently. It is not better if these are moved into one change. The other patch D144273 is not finished (it can get bigger), and a test for this change is needed. I try still to discover what the exact problem is here, then I can add an unit test.

balazske updated this revision to Diff 501161.Feb 28 2023, 8:35 AM

Added a test case.

balazske edited the summary of this revision. (Show Details)Feb 28 2023, 8:42 AM

Basically LGTM (assuming that the TC passes), I added two minor suggestions, but I'm not opposed to merging this in its current state.

clang/unittests/AST/ASTImporterTest.cpp
8142–8150

Nitpick: consider saving this string in a constant instead of repeating it twice.

8201–8202

For clarity, perhaps also assert that Spec1 != Spec2.

balazske retitled this revision from [clang[[ASTImporter] Import TemplateName correctly to [clang][ASTImporter] Import TemplateName correctly.Mar 1 2023, 1:44 AM
balazske updated this revision to Diff 501539.Mar 1 2023, 8:54 AM

Updated the test.

This revision is now accepted and ready to land.Mar 2 2023, 7:32 AM
donat.nagy accepted this revision.Mar 2 2023, 10:05 AM

LGTM as well.

balazske marked 2 inline comments as done.Mar 2 2023, 11:45 PM
This revision was landed with ongoing or failed builds.Mar 3 2023, 12:17 AM
This revision was automatically updated to reflect the committed changes.