Page MenuHomePhabricator

[ASTImporter] Add support for importing fixed point literals
ClosedPublic

Authored by vabridgers on Apr 8 2020, 5:12 AM.

Details

Summary

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

Diff Detail

Event Timeline

vabridgers created this revision.Apr 8 2020, 5:12 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
vabridgers added a comment.EditedApr 8 2020, 5:14 AM

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 added inline comments.
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).

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

vabridgers updated this revision to Diff 256131.Apr 8 2020, 3:54 PM

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

vabridgers updated this revision to Diff 256133.Apr 8 2020, 3:57 PM

Remove extraneous code :/

balazske added inline comments.Apr 9 2020, 12:33 AM
clang/include/clang/AST/Expr.h
1494

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

vabridgers updated this revision to Diff 256234.Apr 9 2020, 3:29 AM

Address comment from @balazske

martong accepted this revision.Apr 9 2020, 7:55 AM

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" }
This revision is now accepted and ready to land.Apr 9 2020, 7:55 AM
vabridgers marked an inline comment as done.Apr 9 2020, 10:17 AM

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

Incorporate Gabor's suggestion for improving test coverage

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

martong accepted this revision.Apr 15 2020, 7:56 AM

Looks good! Thanks!

Closed by commit rG789215dc0db1: [ASTImporter] Add support for importing fixed point literals (authored by vabridgers, committed by einvbri <vince.a.bridgers@ericsson.com>). · Explain WhyApr 15 2020, 8:45 AM
This revision was automatically updated to reflect the committed changes.