Page MenuHomePhabricator

[ASTImporter] Unify redecl chain tests as type parameterized tests
ClosedPublic

Authored by martong on Jan 25 2019, 6:03 AM.

Details

Summary

This patch unifies all those tests which check the correctness of the
redecl chains. Previously we had several structurally very similar test
cases for each language construct (class, function, variable, function
template, ...).

We still use value-parameterized tests for the different AST
compatibility switches (-fdelayed-template-parsing, -fms-compatibility).
Gtest makes it possible to have either value-parameterized or
type-parameterized fixtures. However, we cannot have both value- and
type-parameterized test fixtures. So we use a value-parameterized test
fixture in the gtest sense. We intend to mimic gtest's type-parameters
via the type template parameter. We manually instantiate the different
tests with the each types.

After this patch I am planning to put the "generic redecl chain" related
tests into their own separate test file (in another patch).

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Jan 25 2019, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 5:01 AM

Ping.
Please raise your objections if you have any until the 18th of February (that date I am going to commit if there are no objections). Also, please let me know if you find this deadline too strict.

Hi Gabor,

The refactoring looks great. I have some minor comments inline.

unittests/AST/ASTImporterTest.cpp
3549 ↗(On Diff #183527)

I would ask you not to use numbers when possible: these names are hard to keep in mind. There can be a nice naming scheme like "FromTUDef/FromTUProto/FromTUProtoDef|, I think it would be much better.

3704 ↗(On Diff #183527)

Is it possible to disable the tests instead of commenting them out?

martong updated this revision to Diff 186445.Feb 12 2019, 6:00 AM
  • Remove numbers when possible
  • Use disabled tests instead of commented out tests
martong marked 4 inline comments as done.Feb 12 2019, 6:03 AM

Thanks for the review Alexei!

unittests/AST/ASTImporterTest.cpp
3549 ↗(On Diff #183527)

Ok. I have removed the numbers where it makes sense: where we have a proto and a definition. However, there are cases when we import two similar nodes (e.g. two prototypes) then I think the numbering is needed to identify the first and second node.

3704 ↗(On Diff #183527)

Okay, I have changed the macro so we can disable the tests.

a_sidorin accepted this revision.Feb 16 2019, 1:54 AM

Thanks for the changes! The patch looks completely fine to me now.

This revision is now accepted and ready to land.Feb 16 2019, 1:54 AM
martong updated this revision to Diff 187206.Feb 18 2019, 3:02 AM
martong marked 2 inline comments as done.

Rebase to master

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2019, 3:09 AM