Page MenuHomePhabricator

[ASTImporter] Add test helper Fixture

Authored by martong on Mar 1 2018, 1:39 PM.



Add a helper test Fixture, so we can add tests which can check internal attributes of AST nodes like getPreviousDecl(), isVirtual(), etc.
This enables us to check if a redeclaration chain is correctly built during import, if the virtual flag is preserved during import, etc. We cannot check such attributes with the existing testImport.
Also, this fixture makes it possible to import from several "From" contexts.

We also added several test cases here, some of them are disabled.
We plan to pass the disabled tests in other patches.

Diff Detail

rC Clang

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This comment was removed by martong.
martong retitled this revision from Add test helper Fixture to [ASTImporter] Add test helper Fixture.Mar 1 2018, 1:42 PM
martong edited the summary of this revision. (Show Details)
martong edited the summary of this revision. (Show Details)Mar 1 2018, 1:45 PM
martong edited the summary of this revision. (Show Details)Mar 5 2018, 2:09 AM
martong edited the summary of this revision. (Show Details)Mar 5 2018, 2:12 AM

Hello Gabor,
Unfortunately, we did same or similar work: D44079. We have to decide what approach is going to be used.
It doesn't mean that we should select only one - probably, we can use both approaches together and refactor or change our patches correspondingly.

Hi Aleksei,

I agree that there are some overlapping functionalities in the patches.
As far as I see both make it possible to write tests which import from multiple contexts and I think that is the only redundant part. So, I agree, for now we could use both approaches simultaneously (i.e. merging both patches).

I think the major strength of this patch is that it makes possible to access the whole API of the AST.
However, with testImportSequence in the other patch it is not clear how could we access the internal attributes of the different AST nodes.
In Ericsson, we have several fixes and other tests which depend on this Fixture and without it we can't really make them open source.

I think having both kinds of tests might make sense.
Overall, this looks good to me. Some nits inline.


I do not like the name of this class. It is like calling a variable as variable.


Missing period on the end of the comment and missing space at the beginning.


I think it might worth to have a note why do we use a list and not a vector.


Repeated type, use auto. Same below.
Are we sure these cast's will not fail? Shouldn't we use dynamic casts and asserts?


I wonder if pair or tuple is the better choice here. I have no strong preference just wondering.


Maybe StringRef for the Identifier?


Maybe declaring in the same line would be better.


I prefer to capture FileName by value. Param names should be upper case. Same below.


Formatting is off here.


Capture From as value.


Redundant new lines, same in most of the tests' beginning.


Repeated type.


Repeated type.


Repeated type.


Missing periods in comments.


Repeated type.


Repeated type.

martong updated this revision to Diff 137563.Mar 8 2018, 6:43 AM
martong marked 15 inline comments as done.

Fix code review comments.

martong added inline comments.Mar 8 2018, 6:44 AM

Actually, we set up the static type in buildASTFromCodeWithArgs to OverlayFileSystem. Still you are right it would be better to use dynamic_cast but we can't since clang and the tests are build with -fno-rtti.


I chose tuple here, because it is more general then pair, i.e. in the future we might be able to extend this easier. But this is a weak argument, nevertheless I cannot come up anything besides the pair.

Hello Gabor,

Sorry for the delay with review: the patch is pretty big and it was hard to find time for reviewing it.
I like the patch but it requires some cleanup like formatting changes. Could you please clang-format new code?
Also, I'd like to avoid code duplication. I pointed some places where it happens.


context_s_; two spaces before 'In'.


Could you refactor testImport() to use createVirtualFile() to avoid code duplication?


Must not be called?


This magic constant was factored out in my review.


This means that testing under MSVC is omitted. It caused a lot of buildbot headache before, so I strongly recommend to add it.






Could you please format inline code?


Sometimes we start raw literal from the beginning of the line, sometimes - with indent. Is there any common style?


Code duplication with upper example. In my patch, I introduced a matcher for this; I think it can be reused here.


This can be written on single line.


It looks like this piece of code duplicates below several times. If so, I think it should be factored into a separate function.


This Pattern is repeated often.


FunctionDecl *DefinitionD (same upper).


This fits 80-char limit, no need to split.


We can use ASTContext & directly instead of const Decl *.


Member names should start with capital: Count.


This will cause confusing miscompile if AMatcher is not a BindableMatcher - like matchers defined with AST_MATCHER-like macros, for example. Isn't better to write this call like this:
Finder.addMatcher(BindableMatcher<NodeType>(AMatcher).bind(""), this);

martong updated this revision to Diff 139181.Mar 20 2018, 12:49 PM
martong marked 14 inline comments as done.

Update based on Alexei's comments.

  • Use createVirtualFile in testImport
  • Add minimal style changes
  • Use DeclCounter
  • Add hasFieldOrder matcher
  • Add parameterized tests
martong added inline comments.Mar 20 2018, 12:49 PM

Good point. Changed all tests to be parameterized on the language options.


Yes you are right it is not consistent. I changed everywhere to follow a common style, to start with an indent.


Agree, the new matcher of yours makes this far simpler, changed to use that.


I have refactored the repeating part by using DeclCounter, because what we actually want to ensure is that there is only one decl.


I don't see why is that a problem, could you please elaborate?


This DeclMatcher ought to be a generic purpose matcher which might be used outside of ASTImporterTest.cpp as well.
The D as a parameter provides the generality.
Perhaps if we used this only inside ASTImporterTestBase then we could add another overload which uses the ASTContext... but which one, the "to" or the "from".


I am not sure if this could work. Actually there is no conversion from the BindableMatcher to a DeclarationMatcher.

../../git/llvm/tools/clang/unittests/AST/DeclMatcher.h:37:12: error: no matching member function for call to 'addMatcher'
../../git/llvm/tools/clang/unittests/AST/ASTImporterTest.cpp:1554:57: note: in instantiation of function template specialization 'clang::ast_matchers::DeclMatcher<clang::CXXMethodDecl, clang::ast_matchers::DeclMatcherKind::Last>::match<clang::ast_matchers::internal::BindableMatcher<clang::Decl> >' requested here
  CXXMethodDecl *Def = LastDeclMatcher<CXXMethodDecl>().match(FromTU, Pattern);

Hi Gabor! This looks much better.


In C++, names started with underscore are reserved for implementation so it's undesirable to use them.


I still can see a number of style violations in inline code. Some examples: lack of space between arguments and parameters, no space before '{' in g(){, etc. Could you please fix them?


Tests with has(fieldDecl()) duplicate tests with hasFieldOrder(). Is it intentional?


Should this be match(From, ...) instead of To?


This identifier is very long. How about renaming it to DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder? It is 60 char long instead of 82.


Same as upper.


Please make it a complete sentence.


Please stick * to the name (below too).

martong updated this revision to Diff 139295.Mar 21 2018, 7:31 AM
martong marked 7 inline comments as done.

Add minor syntax changes and rename


Ok, I removed the redundant has(fieldDecl()) checks.


That's right, good catch.


Ok, I like the shorter name, changed it.

martong marked an inline comment as done.Mar 21 2018, 7:32 AM
a.sidorin accepted this revision.Mar 21 2018, 9:35 AM

This looks good to me. Thank you!
@xazax.hun Gabor, could you please take a look?

This revision is now accepted and ready to land.Mar 21 2018, 9:35 AM
xazax.hun accepted this revision.Mar 22 2018, 8:25 AM

Overall looks good, some minor comments inline.


Maybe an IfNeeded suffix or something like that rather than a comment?


When can the TU.Unit be nullptr here? Should this be an assert instead?


I wonder why we only assert on the first one and if we should use GTEST macros here. Same questions below.

martong updated this revision to Diff 139567.Mar 23 2018, 2:40 AM
martong marked 3 inline comments as done.
  • Add some minor changes based on xazax's comments

Added the suffix.


Good catch, changed it.


Changed to use GTEST macros.
The reason why we assert only on the first is this: In the first check we verify the original "from" AST, if the assumption we made on the original AST is false then there is no point to continue. However, in the second check we do verify the logic of the importer which may fail. But that is not a fatal failure, because if there were any other checks after that we would like to know if those checks pass or not.
The main difference between EXPECT_* and ASSERT_* is that ASSERT_* will terminate the given test if it fails, but EXPECT_* keep on going and subsequent checks will be executed in the same test.

Hi Aleksei,
I don't have commit permissions, could you please help and commit?

martong updated this revision to Diff 139791.Mar 26 2018, 7:57 AM
  • Fix gcc 5 warnings


Could someone with commit rights please commit this? @szepet or @xazax.hun ?

This revision was automatically updated to reflect the committed changes.