Page MenuHomePhabricator

[ASTImporter] Add test helper Fixture
ClosedPublic

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

Details

Summary

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

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

unittests/AST/ASTImporterTest.cpp
152

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

157

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

172

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

179

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

193

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

196

Maybe StringRef for the Identifier?

199

Maybe declaring in the same line would be better.

236

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

240

Formatting is off here.

262

Capture From as value.

1024

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

1034

Repeated type.

1061

Repeated type.

1090

Repeated type.

1119

Missing periods in comments.

1184

Repeated type.

1226

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
unittests/AST/ASTImporterTest.cpp
179

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.

193

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.

unittests/AST/ASTImporterTest.cpp
176

context_s_; two spaces before 'In'.

187

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

203

Must not be called?

206

This magic constant was factored out in my review.

207

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

211

FromTU?

235

FoundDecls.front()

998

Could you please format inline code?

1110

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

1213

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

1274

This can be written on single line.

1385

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

1463

This Pattern is repeated often.

1514

FunctionDecl *DefinitionD (same upper).

1546

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

unittests/AST/DeclMatcher.h
34

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

49

Member names should start with capital: Count.

60

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
unittests/AST/ASTImporterTest.cpp
207

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

1110

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

1213

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

1385

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

1463

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

unittests/AST/DeclMatcher.h
34

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

60

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'
    Finder.addMatcher(internal::BindableMatcher<NodeType>(AMatcher).bind(""),
    ~~~~~~~^~~~~~~~~~
../../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.

unittests/AST/ASTImporterTest.cpp
998

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

1001

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?

1167

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

1169

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

1175

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

1192

Same as upper.

1209

Please make it a complete sentence.

1256

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

unittests/AST/ASTImporterTest.cpp
1167

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

1169

That's right, good catch.

1175

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.

unittests/AST/ASTImporterTest.cpp
276

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

289

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

1043

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
unittests/AST/ASTImporterTest.cpp
276

Added the suffix.

289

Good catch, changed it.

1043

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

Ping.

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

This revision was automatically updated to reflect the committed changes.