[PATCH] D77721: [ASTImporter] Add support for importing fixed point literals

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Hi Vince, this looks good to me!

On the other hand I was pondering on @balazske's comment, this one:

> Or use the new flag added to every item in DefaultTestValuesForRunOptions, specially if there is relevant difference in the AST for the options in DefaultTestValuesForRunOptions in the code of this test (probably not).

So, the below test instantiation checks only with the "-ffixed-point" option passed to the frontend.

  INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFixedPointExpr,
                          ::testing::Values(ArgVector{"-ffixed-point"}), );

However, almost all other tests are exercised with a bunch of different options that are listed in `DefaultTestValuesForRunOptions`. We have this mechanism, because previously we had a lot of failures from windows build bots, where the base AST can be different. Of course it is not super relevant to a literal, but with structs and templates it is.

So, I think it would be really useful in the future, if we could extend `DefaultTestValuesForRunOptions` with an additional option. And it is okay to do that in a follow up patch (maybe I'll do it myself, just let me know if you don't have time for that). Here is what I've been thinking:

  --- a/clang/unittests/AST/ASTImporterFixtures.h
  +++ b/clang/unittests/AST/ASTImporterFixtures.h
  @@ -66,10 +66,13 @@ protected:
     }
   };
  
  -const auto DefaultTestValuesForRunOptions = ::testing::Values(
  +const auto DefaultTestArrayForRunOptions = std::array<ArgVector, 4>{
       ArgVector(), ArgVector{"-fdelayed-template-parsing"},
       ArgVector{"-fms-compatibility"},
  -    ArgVector{"-fdelayed-template-parsing", "-fms-compatibility"});
  +    ArgVector{"-fdelayed-template-parsing", "-fms-compatibility"}};
  +
  +const auto DefaultTestValuesForRunOptions =
  +    ::testing::ValuesIn(DefaultTestArrayForRunOptions);



  --- a/clang/unittests/AST/ASTImporterTest.cpp
  +++ b/clang/unittests/AST/ASTImporterTest.cpp
  @@ -5922,6 +5922,28 @@ TEST_P(ASTImporterOptionSpecificTestBase, ImportExprOfAlignmentAttr) {
     EXPECT_TRUE(ToA);
   }
  
  +template <typename T>
  +auto ExtendWithOptions(const T& Values, const ArgVector& Args) {
  +  auto Copy = Values;
  +  for (ArgVector& ArgV : Copy) {
  +    for (const std::string& Arg : Args) {
  +      ArgV.push_back(Arg);
  +    }
  +  }
  +  return ::testing::ValuesIn(Copy);
  +}
  +
  +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFixedPointExpr,
  +                        ExtendWithOptions(DefaultTestArrayForRunOptions,
  +                                          ArgVector{"-ffixed-point"}), );

This would give us the following test instances:

  ParameterizedTests/ImportFixedPointExpr.ImportFixedPointerLiteralExpr/0, where GetParam() = { "-ffixed-point" }
  ParameterizedTests/ImportFixedPointExpr.ImportFixedPointerLiteralExpr/1, where GetParam() = { "-fdelayed-template-parsing", "-ffixed-point" }
  ParameterizedTests/ImportFixedPointExpr.ImportFixedPointerLiteralExpr/2, where GetParam() = { "-fms-compatibility", "-ffixed-point" }
  ParameterizedTests/ImportFixedPointExpr.ImportFixedPointerLiteralExpr/3, where GetParam() = { "-fdelayed-template-parsing", "-fms-compatibility", "-ffixed-point" }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77721/new/

https://reviews.llvm.org/D77721