[ASTImporter] Add support for importing fixed point literals

[vabridgers]

Summary

This patch adds support for importing fixed point literals, following
up to https://reviews.llvm.org/D46915 specifically for importing AST.

[vabridgers]

Landing this change depends on https://reviews.llvm.org/D57226 to be pushed. Please review for now, and I'll be sure to push this only after https://reviews.llvm.org/D57226 is pushed. Thanks!

[balazske]

clang/unittests/AST/ASTImporterTest.cpp

1009

See test ImportFloatinglLiteralExpr for a better implementation of this test. The new test could be included after ImportFloatinglLiteralExpr. But a new base class is needed like struct ImportFixedPointExpr : ImportExpr {};.

5953

The default options should not be changed for every (...) test as this change does that. Something like this is better:

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

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

[vabridgers]

@balazske, Thank you for the comments. I'll address and repost the review.

[vabridgers]

Addressed comments from @balazske.
Thanks for the tips and useful starting point from @martong

[vabridgers]

Remove extraneous code :/

[balazske]

clang/include/clang/AST/Expr.h

1494 ↗

(On Diff #256133)

This line removal is not needed and can cause disturbances, it is better to remove it.

[vabridgers]

Address comment from @balazske

[martong]

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

[vabridgers]

Ahhh yes, I see. I can get this done while we're waiting on https://reviews.llvm.org/D57226 to land. Thanks Gabor!!

[vabridgers]

Incorporate Gabor's suggestion for improving test coverage

[vabridgers]

Do these changes look ok to land? https://reviews.llvm.org/D57226 is pushed. Thanks!

[martong]

Looks good! Thanks!

clang/include/clang/ASTMatchers/ASTMatchers.h

 extern const internal::VariadicDynCastAllOfMatcher<Stmt, FloatingLiteral>
     floatLiteral;
 
 /// Matches imaginary literals, which are based on integer and floating
 /// point literals e.g.: 1i, 1.0i
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, ImaginaryLiteral>
     imaginaryLiteral;
 
+/// Matches fixed point literals
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, FixedPointLiteral>
+    fixedPointLiteral;
+
 /// Matches user defined literal operator call.
 ///
 /// Example match: "foo"_suffix
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, UserDefinedLiteral>
     userDefinedLiteral;
 
 /// Matches compound (i.e. non-scalar) literals
 ///

clang/lib/AST/ASTImporter.cpp

public:
     ExpectedStmt VisitPredefinedExpr(PredefinedExpr *E);
     ExpectedStmt VisitDeclRefExpr(DeclRefExpr *E);
     ExpectedStmt VisitImplicitValueInitExpr(ImplicitValueInitExpr *E);
     ExpectedStmt VisitDesignatedInitExpr(DesignatedInitExpr *E);
     ExpectedStmt VisitCXXNullPtrLiteralExpr(CXXNullPtrLiteralExpr *E);
     ExpectedStmt VisitIntegerLiteral(IntegerLiteral *E);
     ExpectedStmt VisitFloatingLiteral(FloatingLiteral *E);
     ExpectedStmt VisitImaginaryLiteral(ImaginaryLiteral *E);
+    ExpectedStmt VisitFixedPointLiteral(FixedPointLiteral *E);
     ExpectedStmt VisitCharacterLiteral(CharacterLiteral *E);
     ExpectedStmt VisitStringLiteral(StringLiteral *E);
     ExpectedStmt VisitCompoundLiteralExpr(CompoundLiteralExpr *E);
     ExpectedStmt VisitAtomicExpr(AtomicExpr *E);
     ExpectedStmt VisitAddrLabelExpr(AddrLabelExpr *E);
     ExpectedStmt VisitConstantExpr(ConstantExpr *E);
     ExpectedStmt VisitParenExpr(ParenExpr *E);
     ExpectedStmt VisitParenListExpr(ParenListExpr *E);
ExpectedStmt ASTNodeImporter::VisitImaginaryLiteral(ImaginaryLiteral *E) {
   ExpectedExpr ToSubExprOrErr = import(E->getSubExpr());
   if (!ToSubExprOrErr)
     return ToSubExprOrErr.takeError();
 
   return new (Importer.getToContext()) ImaginaryLiteral(
       *ToSubExprOrErr, *ToTypeOrErr);
 }
 
+ExpectedStmt ASTNodeImporter::VisitFixedPointLiteral(FixedPointLiteral *E) {
+  auto ToTypeOrErr = import(E->getType());
+  if (!ToTypeOrErr)
+    return ToTypeOrErr.takeError();
+
+  ExpectedSLoc ToLocationOrErr = import(E->getLocation());
+  if (!ToLocationOrErr)
+    return ToLocationOrErr.takeError();
+
+  return new (Importer.getToContext()) FixedPointLiteral(
+      Importer.getToContext(), E->getValue(), *ToTypeOrErr, *ToLocationOrErr,
+      Importer.getToContext().getFixedPointScale(*ToTypeOrErr));
+}
+
 ExpectedStmt ASTNodeImporter::VisitCharacterLiteral(CharacterLiteral *E) {
   ExpectedType ToTypeOrErr = import(E->getType());
   if (!ToTypeOrErr)
     return ToTypeOrErr.takeError();
 
   ExpectedSLoc ToLocationOrErr = import(E->getLocation());
   if (!ToLocationOrErr)
     return ToLocationOrErr.takeError();

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

const internal::VariadicDynCastAllOfMatcher<Stmt, CXXBoolLiteralExpr>
     cxxBoolLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, StringLiteral> stringLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CharacterLiteral>
     characterLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, IntegerLiteral>
     integerLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, FloatingLiteral> floatLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, ImaginaryLiteral> imaginaryLiteral;
+const internal::VariadicDynCastAllOfMatcher<Stmt, FixedPointLiteral>
+    fixedPointLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, UserDefinedLiteral>
     userDefinedLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CompoundLiteralExpr>
     compoundLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CXXNullPtrLiteralExpr>
     cxxNullPtrLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, ChooseExpr> chooseExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, GNUNullExpr> gnuNullExpr;

clang/unittests/AST/ASTImporterTest.cpp

 static const RecordDecl *getRecordDeclOfFriend(FriendDecl *FD) {
   QualType Ty = FD->getFriendType()->getType().getCanonicalType();
   return cast<RecordType>(Ty)->getDecl();
 }
 
 struct ImportExpr : TestImportBase {};
 struct ImportType : TestImportBase {};
 struct ImportDecl : TestImportBase {};
+struct ImportFixedPointExpr : ImportExpr {};
 
 struct CanonicalRedeclChain : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(CanonicalRedeclChain, ShouldBeConsequentWithMatchers) {
   Decl *FromTU = getTuDecl("void f();", Lang_CXX);
   auto Pattern = functionDecl(hasName("f"));
   auto *D0 = FirstDeclMatcher<FunctionDecl>().match(FromTU, Pattern);
 
testImport(
           floatLiteral(equals(1.0), hasType(asString("double"))))));
   testImport(
       "void declToImport() { (void)1.0e-5f; }",
       Lang_C, "", Lang_C, Verifier,
       functionDecl(hasDescendant(
           floatLiteral(equals(1.0e-5f), hasType(asString("float"))))));
 }
 
+TEST_P(ImportFixedPointExpr, ImportFixedPointerLiteralExpr) {
+  MatchVerifier<Decl> Verifier;
+  testImport("void declToImport() { (void)1.0k; }", Lang_C, "", Lang_C,
+             Verifier, functionDecl(hasDescendant(fixedPointLiteral())));
+  testImport("void declToImport() { (void)0.75r; }", Lang_C, "", Lang_C,
+             Verifier, functionDecl(hasDescendant(fixedPointLiteral())));
+}
+
 TEST_P(ImportExpr, ImportImaginaryLiteralExpr) {
   MatchVerifier<Decl> Verifier;
   testImport(
       "void declToImport() { (void)1.0i; }",
       Lang_CXX14, "", Lang_CXX14, Verifier,
       functionDecl(hasDescendant(imaginaryLiteral())));
 }
 
Decl *FromTU = getTuDecl(
       "int declToImport(){ return NONAME_SIZEOF(int); }",
       Lang_C, "input.c");
   auto *From = FirstDeclMatcher<FunctionDecl>().match(
       FromTU, functionDecl(hasName("declToImport")));
   ASSERT_TRUE(From);
   auto *To = Import(From, Lang_C);
   ASSERT_TRUE(To);
   EXPECT_TRUE(MatchVerifier<FunctionDecl>().match(
       To, functionDecl(hasName("declToImport"),
                        hasDescendant(unaryExprOrTypeTraitExpr()))));
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
        ImportRecordDeclInFuncParamsFromMacro) {
   // This construct is not supported by ASTImporter.
   Decl *FromTU = getTuDecl(
       "#define PAIR_STRUCT(type) struct data_t{type a;type b;} \n"
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFixedPointExpr,
+                        ::testing::Values(ArgVector{"-ffixed-point"}), );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportType,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportDecl,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterOptionSpecificTestBase,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, RedirectingImporterTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,