This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Add ms compatibility to tests
ClosedPublic

Authored by martong on May 25 2018, 4:46 AM.

Diff Detail

Repository
rC Clang

Event Timeline

martong created this revision.May 25 2018, 4:46 AM

This patch does not target testImport because I am not sure how to handle the options there:

auto RunOptsFrom = getRunOptionsForLanguage(FromLang);
auto RunOptsTo = getRunOptionsForLanguage(ToLang);
for (const auto &FromArgs : RunOptsFrom)
  for (const auto &ToArgs : RunOptsTo)
    EXPECT_TRUE(testImport(FromCode, FromArgs, ToCode, ToArgs,
                           Verifier, AMatcher));

I think, it is overkill to test all possible combinations of the options, we should come up with something better here.
Perhaps we could exploit parameterized tests here as well. @a.sidorin, @xazax.hun What is your opinion?

I think, it is overkill to test all possible combinations of the options, we should come up with something better here.

I agree with that. I think we need to test just import pairs {/*From*/no_option, /*To*/no_option}, {option_1, option1}, {option_2, option_2}, ...{option_n, option_n}.
Another option is to just turn -fno-delayed-template-parsing -fno-ms-compatibility for ASTImporter tests like it is done in some unit tests, but I'm not sure it's a correct solution.

I agree with that. I think we need to test just import pairs {/*From*/no_option, /*To*/no_option}, {option_1, option1}, {option_2, option_2}, ...{option_n, option_n}.

This patch does exactly that with the parameterized tests. Each elements of the DefaultTestValuesForRunOptions will be used both for the "To" and for the "From" context.
So we will have each TEST_P generating 4 cases:
[FROM, TO]
{no_option, no_option}
{"-fdelayed-template-parsing", "-fdelayed-template-parsing"}
{"-fms-compatibility", "-fms-compatibility"}
{"-fdelayed-template-parsing -fms-compatibility", "-fdelayed-template-parsing -fms-compatibility"}

I meant that we can use this approach for testImport() too.

martong updated this revision to Diff 149093.May 30 2018, 5:00 AM
  • Moved the family of testImport functions under a test fixture class, so we can use parameterized test.
  • Refactored testImport and testImportSequence, because for loops over the different compiler options is no longer needed, that is handeld by the test framework via parameters from now on.
martong updated this revision to Diff 149094.May 30 2018, 5:05 AM
  • Forgot to instantiate some of the parameterized tests
martong retitled this revision from [ASTImporter] Add ms compatibility to tests which use the TestBase to [ASTImporter] Add ms compatibility to tests.May 30 2018, 5:06 AM

Balazs, could you please review this patch as well? (This code is not in our fork yet.)

martong edited the summary of this revision. (Show Details)May 30 2018, 5:19 AM
martong updated this revision to Diff 149095.May 30 2018, 5:28 AM
  • Remove unused function
martong updated this revision to Diff 149096.May 30 2018, 5:31 AM
  • Remove unused RunOptions typedef and isCXX function
balazske accepted this revision.May 30 2018, 6:31 AM

Found no big problems. But not all extra options are applicable to all languages (template related things to C) so there may be redundant tests.

This revision is now accepted and ready to land.May 30 2018, 6:31 AM
martong requested review of this revision.Jun 19 2018, 8:23 AM

Ping.

a_sidorin accepted this revision.Jun 24 2018, 3:36 PM
a_sidorin added a subscriber: a_sidorin.

LGTM, thank you!

Could you describe the refactoring in the commit message?

unittests/AST/ASTImporterTest.cpp
182

Looks like clang-format broke this comment in some places.

This revision is now accepted and ready to land.Jun 24 2018, 3:36 PM
martong updated this revision to Diff 152674.Jun 25 2018, 6:03 AM
  • Update commit comment and fix broken format in a comment.
martong marked an inline comment as done.Jun 25 2018, 6:03 AM
This revision was automatically updated to reflect the committed changes.