Page MenuHomePhabricator

[ASTImporter] Support importing FunctionTemplateDecl and CXXDependentScopeMemberExpr
ClosedPublic

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

Diff Detail

Repository
rL LLVM

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
3906 ↗(On Diff #118214)

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
3906 ↗(On Diff #118214)

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
3858 ↗(On Diff #118214)

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

3901 ↗(On Diff #118214)

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.

5429 ↗(On Diff #118214)

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

5434 ↗(On Diff #118214)

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

5438 ↗(On Diff #118214)

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

5462 ↗(On Diff #118214)

Should we fail if nullptr was returned?

unittests/AST/ASTImporterTest.cpp
489 ↗(On Diff #118214)

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
3901 ↗(On Diff #118214)

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
5462 ↗(On Diff #118214)

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 ↗(On Diff #124087)

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
5835 ↗(On Diff #124121)

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
5835 ↗(On Diff #124868)

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
559
  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.