This patch introduces the ability to test an arbitrary sequence of imports between a given set of virtual source files. This should finally allow us to write simple tests and fix annoying issues inside ASTImporter that cause failures in CSA CTU. This is done by refactoring ASTImporterTest functions and introducing `testImportSequence` facility. As a side effect, `testImport` facility was generalized a bit more. It should now allow import of non-decl AST nodes; however, there is still no test using this ability. As a "test for test", there is also a fix for import anonymous TagDecls referred by typedef. Before this patch, the setting of typedef for anonymous structure was delayed; however, this approach misses the corner case if an enum constant is imported directly. In this patch, typedefs for anonymous declarations are imported right after the anonymous declaration is imported, without any delay.
Details
- Reviewers
r.stahl xazax.hun jingham szepet - Commits
- rG04fbffcc52fa: [ASTImporter] Allow testing of import sequences; fix import of typedefs for…
rC330704: [ASTImporter] Allow testing of import sequences; fix import of typedefs for…
rL330704: [ASTImporter] Allow testing of import sequences; fix import of typedefs for…
Diff Detail
- Repository
- rL LLVM
Event Timeline
Hi,
Thank you for the patch.
It seems that the new approach still does not solve a problem with anonymous structures in typedefs. In C++ a copy constructor is automatically generated, and its parameter is the anonymous structure itself. This triggers caching NoLinkage for it during the import of the constructor, but later fails with an assert because at the end the computed linkage is ExternalLinkage. It seems that anonymous structures are somewhat tricky in the original AST. For this struct:
typedef struct { int n; } T;
The original AST is:
CXXRecordDecl 0x1abcb38 </tmp/first.cpp:1:9, line:3:1> line:1:9 imported struct definition |-FieldDecl 0x1abce68 <line:2:3, col:7> col:7 imported n 'int' |-CXXConstructorDecl 0x1abced0 <line:1:9> col:9 imported implicit used 'void (void) throw()' inline default trivial | `-CompoundStmt 0x1abd2e0 <col:9> `-CXXConstructorDecl 0x1abcfd8 <col:9> col:9 imported implicit 'void (const T &)' inline default trivial noexcept-unevaluated 0x1abcfd8 `-ParmVarDecl 0x1abd138 <col:9> col:9 imported 'const T &' TypedefDecl 0x1abcc78 </tmp/first.cpp:1:1, line:3:3> col:3 imported referenced T 'struct T':'T' `-ElaboratedType 0x1abccd0 'struct T' sugar imported `-RecordType 0x1abcc50 'T' imported `-CXXRecord 0x1abcb38 ''
But the imported one is:
CXXRecordDecl 0x1a51400 </tmp/first.cpp:1:9> col:9 struct definition |-FieldDecl 0x1a51540 <line:2:3, col:7> col:7 n 'int' |-CXXConstructorDecl 0x1a515e0 <line:1:9> col:9 implicit used 'void (void) throw()' inline trivial | `-CompoundStmt 0x1a51688 <col:9> `-CXXConstructorDecl 0x1a51768 <col:9> col:9 implicit 'void (const struct (anonymous at /tmp/first.cpp:1:9) &)' inline trivial noexcept-unevaluated 0x1a51768 `-ParmVarDecl 0x1a51708 <col:9> col:9 'const struct (anonymous at /tmp/first.cpp:1:9) &' TypedefDecl 0x1a518b0 </tmp/first.cpp:1:1, line:3:3> col:3 T 'struct (anonymous struct at /tmp/first.cpp:1:9)':'struct (anonymous at /tmp/first.cpp:1:9)' `-ElaboratedType 0x1a51860 'struct (anonymous struct at /tmp/first.cpp:1:9)' sugar `-RecordType 0x1a514a0 'struct (anonymous at /tmp/first.cpp:1:9)' `-CXXRecord 0x1a51400 ''
By moving the code that sets the type name of an anoynmous declaration from Import(Decl*) to ImportDefinition(RecordDecl*, RecordDecl*, ImportDefinitionKind) (and the same for Enum) we will not crash upon importing typedefs containing anonymous strcutures. This is a patch-over-patch for it, including the test cases:
Overall looks good, some minor nits inline. If those can be resolved trivially, I am ok with committing this without another round of reviews.
unittests/AST/ASTImporterTest.cpp | ||
---|---|---|
91 ↗ | (On Diff #141537) | If we already touching this part, maybe better to take the code as a StringRef? |
356 ↗ | (On Diff #141537) | Maybe using StringRef for DeclName too? |
416 ↗ | (On Diff #141537) | Do we need this? Can't we just say if a filename is in AllASTUnits, it is built? |