This patch adds support for importing two different kind of AST nodes (FunctionTemplateDecl and CXXDependentScopeMemberExpr).
Note: This solution is based on D26904 (which I did not wanted to commandeer right away) which is probably based on (https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp#L7564).
Details
- Reviewers
khazem xazax.hun spyffe a.sidorin bruno - Commits
- rG7f758b6af54b: [ASTImporter] Support importing FunctionTemplateDecl and…
rGdec81835d1d4: [ASTImporter] Support importing FunctionTemplateDecl and…
rC321492: [ASTImporter] Support importing FunctionTemplateDecl and…
rL321492: [ASTImporter] Support importing FunctionTemplateDecl and…
rC320942: [ASTImporter] Support importing FunctionTemplateDecl and…
rL320942: [ASTImporter] Support importing FunctionTemplateDecl and…
Diff Detail
Event Timeline
| lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 4281 | Do you need to set the access here? Isn't there some code that is triggered earlier and set this? | |
| lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 4281 | Unfortunately no, it has to be set manually. | |
Some comments inline.
| lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 4233 | DC is imported first so it is possible to have a failed import and non-null DC. The assertion here seems to be incorrect. | |
| 4276 | I think this should be placed into a separate function: there are many cases that can require attribute import so this code can be reused. | |
| 5815 | This will assert if isImplicit() method returns true. In such case, Base should be nullptr. | |
| 5820 | Are any of source parts can be null? Do we need to check them all? | |
| 5824 | I have placed a function template for import of TemplateArgumentListInfo into D38845. | |
| 5848 | Should we fail if nullptr was returned? | |
| unittests/AST/ASTImporterTest.cpp | ||
| 502 | Could you please format the code with indents? | |
| lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 4276 | Actually, I just realized it is part of the ´Importer.Imported´ function call as well, so this is unnecessary. | |
| lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 5848 | Since E->getFirstQualifierFoundInScope() can be null, it does not necessarily mean that it should fail. However, I checked it in a more structured way above if its OK. | |
Hi Peter. All is almost OK but I have some doubts about one of checking conditions.
| lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 5831 | Did you mean _!_E->getMember.isEmpty()? If so, you can rewrite the code as if (E->getMember() && !Name). | |
| lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 5835 | I still have questions about this line. I strongly believe that we should fail if the source DeclarationName is non-empty and the result is empty. This line fails if they are both empty which seems to be a correct case for me. | |
Yepp, I am not exactly sure how I managed to do it, but I've missed this comment last time. Fixed it.
| lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 5835 | Peter, this condition still looks inverted to me. Am I missing something? | |
Nope, that was me. If this (and the dependencies) are looking good, I can commit them as well (added the dependencies ^^).
Peter, could you please fix the tests to correspond new format? I've made them pass with some changes (in inline comment).
| cfe/trunk/unittests/AST/ASTImporterTest.cpp | ||
|---|---|---|
| 519 ↗ | (On Diff #127276) |
|
Sorry for the noise, I have fixed the patch and re-committed it, but forgot to remove "Request changes" status.
DC is imported first so it is possible to have a failed import and non-null DC. The assertion here seems to be incorrect.