Page MenuHomePhabricator

[ASTImporter] Support importing FunctionTemplateDecl and CXXDependentScopeMemberExpr
ClosedPublic

Authored by szepet on Oct 9 2017, 9:08 AM.

Diff Detail

Event Timeline

szepet created this revision.Oct 9 2017, 9:08 AM
xazax.hun added inline comments.Oct 19 2017, 2:49 AM
lib/AST/ASTImporter.cpp
4283

Do you need to set the access here? Isn't there some code that is triggered earlier and set this?

szepet marked an inline comment as done.Oct 19 2017, 6:03 AM
szepet added inline comments.
lib/AST/ASTImporter.cpp
4283

Unfortunately no, it has to be set manually.

dkrupp added a reviewer: bruno.Nov 3 2017, 8:50 AM
a.sidorin edited edge metadata.Nov 13 2017, 9:06 AM

Some comments inline.

lib/AST/ASTImporter.cpp
4235

DC is imported first so it is possible to have a failed import and non-null DC. The assertion here seems to be incorrect.

4278

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.

5817

This will assert if isImplicit() method returns true. In such case, Base should be nullptr.

5822

Are any of source parts can be null? Do we need to check them all?

5826

I have placed a function template for import of TemplateArgumentListInfo into D38845.

5850

Should we fail if nullptr was returned?

unittests/AST/ASTImporterTest.cpp
502

Could you please format the code with indents?

szepet updated this revision to Diff 124082.Nov 23 2017, 7:26 AM
szepet marked 8 inline comments as done.

Updates based on comments.

szepet added inline comments.Nov 23 2017, 7:26 AM
lib/AST/ASTImporter.cpp
4278

Actually, I just realized it is part of the ´Importer.Imported´ function call as well, so this is unnecessary.

szepet updated this revision to Diff 124087.Nov 23 2017, 7:44 AM

Left some unformatted lines in the previous version. Fixed.

szepet added inline comments.Nov 23 2017, 7:46 AM
lib/AST/ASTImporter.cpp
5850

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
5833

Did you mean _!_E->getMember.isEmpty()? If so, you can rewrite the code as if (E->getMember() && !Name).

szepet updated this revision to Diff 124121.Nov 23 2017, 4:32 PM

Updating the usage of ImportTemplateArgumentListInfo.

a.sidorin added inline comments.Nov 26 2017, 5:23 AM
lib/AST/ASTImporter.cpp
5837

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.

szepet updated this revision to Diff 124868.Nov 29 2017, 7:23 PM
szepet marked an inline comment as done.

Yepp, I am not exactly sure how I managed to do it, but I've missed this comment last time. Fixed it.

a.sidorin added inline comments.Dec 1 2017, 9:38 AM
lib/AST/ASTImporter.cpp
5837

Peter, this condition still looks inverted to me. Am I missing something?

szepet updated this revision to Diff 125971.Dec 7 2017, 8:38 AM
szepet marked 2 inline comments as done.

Nope, that was me. If this (and the dependencies) are looking good, I can commit them as well (added the dependencies ^^).

LGTM, thank you!

a.sidorin accepted this revision.Dec 7 2017, 9:12 AM
This revision is now accepted and ready to land.Dec 7 2017, 9:12 AM
This revision was automatically updated to reflect the committed changes.
xazax.hun reopened this revision.Dec 20 2017, 5:54 AM

It was reverted due to tests failures on windows build bots.

This revision is now accepted and ready to land.Dec 20 2017, 5:54 AM
a.sidorin requested changes to this revision.Dec 21 2017, 10:39 AM

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)
  1. For these two tests, C should be struct because t is a private member and is inaccessible from declToImport().
  2. An additional line void foo() { declToImport<int>(); } should resolve the issue for both tests.
This revision now requires changes to proceed.Dec 21 2017, 10:39 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Dec 27 2017, 9:05 AM
This revision was automatically updated to reflect the committed changes.

Sorry for the noise, I have fixed the patch and re-committed it, but forgot to remove "Request changes" status.