Page MenuHomePhabricator

[ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr
ClosedPublic

Authored by szepet on Oct 12 2017, 6:13 AM.

Diff Detail

Event Timeline

szepet created this revision.Oct 12 2017, 6:13 AM
xazax.hun added inline comments.Oct 19 2017, 2:33 AM
lib/AST/ASTImporter.cpp
5500

According to phabricator this code is very similar to a snippet starting from line 4524 and some code bellow. Maybe it would be worth to have a function instead of duplicating?

szepet added inline comments.Oct 19 2017, 5:29 AM
lib/AST/ASTImporter.cpp
5500

Good point, I would do it in a separate patch and add it as a dependency if it is OK.

xazax.hun added inline comments.Oct 19 2017, 5:33 AM
lib/AST/ASTImporter.cpp
5500

Sure, that would be awesome. :)

dkrupp added a reviewer: bruno.Nov 3 2017, 8:49 AM
a.sidorin edited edge metadata.Nov 13 2017, 6:31 AM

Hello Peter,

Looks mostly good, but there are some minor comments.

lib/AST/ASTImporter.cpp
5500

This is a template I have prepared for a patch still non-commited yet:

template<typename InContainerTy>
bool ImportTemplateArgumentListInfo(const InContainerTy &Container,
                                    TemplateArgumentListInfo &ToTAInfo) {
  for (const auto &FromLoc : Container) {
    if (auto ToLoc = ImportTemplateArgumentLoc(FromLoc))
      ToTAInfo.addArgument(*ToLoc);
    else
      return true;
  }
  return false;
}
5626

Needed space after if.

unittests/AST/ASTImporterTest.cpp
574

Could you please align the code as conventions require?

szepet updated this revision to Diff 124119.Nov 23 2017, 4:16 PM
szepet marked 6 inline comments as done.

Updated based on review comments.

Hello Aleksei,
Thank for the review and the code snippet as well!

a.sidorin accepted this revision.Nov 26 2017, 3:52 AM

Hello Peter,

This looks good to me. But could you please check if test works if we add -fms-compatibility and -fdelayed-template-parsing to Args?

unittests/AST/ASTImporterTest.cpp
557

Uninstantiated templates make me worry about Windows buildbots. If they will start to fail, we should test these matchers with -fms-compatibility and -fdelayed-template-parsing options enabled.

This revision is now accepted and ready to land.Nov 26 2017, 3:52 AM
szepet marked an inline comment as done.Nov 29 2017, 7:17 PM
szepet added inline comments.
unittests/AST/ASTImporterTest.cpp
557

Not sure about Windows but I can ensure that this test has passed with these args added as well.

Hello Peter. Please set the dependencies for the patch - it cannot be applied clearly and even if I add ImportTemplateArgumentListInfo, tests still fail - looks like FunctionTemplateDecl patch should be applied first.

This should be rebased to latest master.

Hi Peter,
The changes needed for rebase are too large - they contain both fixes for compilation failures and fixes for test errors. Could you please upgrade the patch?

szepet marked an inline comment as done.Apr 11 2018, 3:27 AM

Hello Aleksei,

Thanks for carrying the ASTImporter improvements (in general)! I will rebase it and upload for sure, but probably just next week. (After EuroLLVM.)

szepet updated this revision to Diff 143925.Apr 25 2018, 7:10 AM

Rewritten the tests using the newly added TEST_P method.
This patch failed earlier when -fdelayed-template-parsing flag was enabled, however, the actual import process was OK but the original AST havent included the checked nodes.
TEST_P made possible to check whether the original code contains the node we would like to import (and we test only in this case, since otherwise it does not make sense).

martong added inline comments.Apr 25 2018, 12:24 PM
unittests/AST/ASTImporterTest.cpp
604

Instead of having 4 FromTUs, perhaps a simpler alternative would be to have only one, which contains all the code snippets with declToImport0 ... declToImport3 (Or just skip declToImport since we no longer need that phabricated name, so we could just have a simple name like X) and importing them one by one later with ASTImporterTestBase::Import.
This would spare the need for the transform and the for cycle below.

617

I think, this if would be needed only if the test suite would be parameterized also with ArgVector{"-fdelayed-template-parsing"}, but that is not.

621

It will be hard to notice based on the test output which DSDRE's import was unsuccessful. Therefore I'd unroll the for loop instead, also please see the above comment that it would be more practical to have one FromTU.

szepet marked an inline comment as done.Apr 26 2018, 4:51 AM
szepet added inline comments.
unittests/AST/ASTImporterTest.cpp
617

Its necessary, since on windows platforms we have that flag by default (so, it does not matter that we dont include here, it will be used anyway).

621

I understand the concern, but I am really against the mentioned design. The main point of this code is that whenever we would like to add one more test cases, then we need to add it to the vector and everything else is done. Other than that, I would say that debugging a broken test case is already a "time expensive" job, so in that case it wouldnt make much of a different, since you need to look at the code closely anyway.

a.sidorin requested changes to this revision.Apr 26 2018, 5:23 AM
a.sidorin added inline comments.
unittests/AST/ASTImporterTest.cpp
617

The declaration should be always present independently of flags so if is invalid here. To achieve this, we use explicit template instantiations in the test code. See ImportCXXDependentScopeMemberExpr test for reference. Explicit instantiations also allow us to find errors in the template code itself.

625

Please don't do this - always test the code with both flag options. Explicit instantiations will help you to do this. Otherwise, windows buildbot will add the flag on its own causing the test to crash after the commit.

This revision now requires changes to proceed.Apr 26 2018, 5:23 AM
szepet updated this revision to Diff 144842.May 2 2018, 2:51 AM
szepet marked an inline comment as done.

Yepp, good point, Aleksei, rewritten the tests using explicit instantiation.

martong accepted this revision.May 2 2018, 4:02 AM

LGTM!

unittests/AST/ASTImporterTest.cpp
553

Perhaps a newline before would make it more independent from the test case instantiation.

martong added inline comments.May 2 2018, 4:03 AM
unittests/AST/ASTImporterTest.cpp
580

Perhaps a newline before would make it more independent from the Matcher, as you did it at line 1544 .

Hello Peter! This looks almost OK, just some minor formatting issues.

unittests/AST/ASTImporterTest.cpp
557

Please add spaces after and before {}.

558

Indentation is a bit off here and in the samples below.

szepet updated this revision to Diff 145159.May 4 2018, 1:02 AM

Format changes added based on comments.

a.sidorin accepted this revision.May 4 2018, 6:12 AM

LGTM!

This revision is now accepted and ready to land.May 4 2018, 6:12 AM
This revision was automatically updated to reflect the committed changes.