[NFC] [clang] Forward forwarding reference

[ccotter]

Summary

Update function bodies to forward forwarding references.
I spotted this while authoring a clang-tidy tool for CppCoreGuideline F.19

[dblaikie]

any chance this is testable, maybe in a unit test?

[ccotter]

Realistically, I'm not sure if this change is all that valuable aside from abiding by the CppCoreGuidelines from the readability and consistency standpoint. The only impact this change would have that I can think of is allowing an object with an && (or other combination) qualified operator() that would be picked up by this change, which doesn't seem likely of a use case for this function. I'll try to add a small test that covers this and leave it to the maintainers whether this is a worthy change :)

[ccotter]

• Add test

[ccotter]

@dblaikie thoughts?

[dblaikie]

Although we may not agree such ideas, we should offer an option for the users to give them the right to choose.

I think @iains was bemoaning the large number of module flags recently - perhaps he'd have some thoughts on this, but mine are that we should be more prescriptive - figure out what a good solution is for most users and implement that, rather than giving people lots of knobs to tune.

For this issue - honestly I think it might be a better/safer default to not import external module definitions at all/ever. Leave it to LTO to do that kind of cross-module optimization, as we have in the past/non-modular builds.

At least until/unless we implement something more like what the bug describes - doing IR level optimizations first and importing the optimized IR, rather than reoptimizing these external definitions in every user.

[dblaikie]

In D143877#4156769, @dblaikie wrote:

Although we may not agree such ideas, we should offer an option for the users to give them the right to choose.

I think @iains was bemoaning the large number of module flags recently - perhaps he'd have some thoughts on this, but mine are that we should be more prescriptive - figure out what a good solution is for most users and implement that, rather than giving people lots of knobs to tune.

For this issue - honestly I think it might be a better/safer default to not import external module definitions at all/ever. Leave it to LTO to do that kind of cross-module optimization, as we have in the past/non-modular builds.

At least until/unless we implement something more like what the bug describes - doing IR level optimizations first and importing the optimized IR, rather than reoptimizing these external definitions in every user.

Huh, this ended up on quite the wrong review... fascinating.

Sorry about that. Disregard. I'll follow up with on-topic review feedback here shortly.

[dblaikie]

Looks good, thanks!

Can you commit this yourself, or would you like me to commit it on your behalf? (what name/email address would you like it attributed to?)

[ccotter]

I don't have commit access - could you use Author: Chris Cotter <[e-mail]>? Thanks!

[ccotter]

Thanks!

clang/include/clang/AST/IgnoreExpr.h

 
 namespace clang {
 namespace detail {
 /// Given an expression E and functions Fn_1,...,Fn_n : Expr * -> Expr *,
 /// Return Fn_n(...(Fn_1(E)))
 inline Expr *IgnoreExprNodesImpl(Expr *E) { return E; }
 template <typename FnTy, typename... FnTys>
 Expr *IgnoreExprNodesImpl(Expr *E, FnTy &&Fn, FnTys &&... Fns) {
-  return IgnoreExprNodesImpl(Fn(E), std::forward<FnTys>(Fns)...);
+  return IgnoreExprNodesImpl(std::forward<FnTy>(Fn)(E),
+                             std::forward<FnTys>(Fns)...);
 }
 } // namespace detail
 
 /// Given an expression E and functions Fn_1,...,Fn_n : Expr * -> Expr *,
 /// Recursively apply each of the functions to E until reaching a fixed point.
 /// Note that a null E is valid; in this case nothing is done.
 template <typename... FnTys> Expr *IgnoreExprNodes(Expr *E, FnTys &&... Fns) {
   Expr *LastE = nullptr;

clang/unittests/AST/ASTExprTest.cpp

+//===- unittests/AST/ASTExprTest.cpp --- AST Expr tests -------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains tests for AST Expr related methods.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ASTPrint.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/IgnoreExpr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+TEST(ASTExpr, IgnoreExprCallbackForwarded) {
+  constexpr char Code[] = "";
+  auto AST = tooling::buildASTFromCodeWithArgs(Code, /*Args=*/{"-std=c++20"});
+  ASTContext &Ctx = AST->getASTContext();
+
+  auto createIntLiteral = [&](uint32_t Value) -> IntegerLiteral * {
+    const int numBits = 32;
+    return IntegerLiteral::Create(Ctx, llvm::APInt(numBits, Value),
+                                  Ctx.UnsignedIntTy, {});
+  };
+
+  struct IgnoreParens {
+    Expr *operator()(Expr *E) & { return nullptr; }
+    Expr *operator()(Expr *E) && {
+      if (auto *PE = dyn_cast<ParenExpr>(E)) {
+        return PE->getSubExpr();
+      }
+      return E;
+    }
+  };
+
+  {
+    auto *IntExpr = createIntLiteral(10);
+    ParenExpr *PE =
+        new (Ctx) ParenExpr(SourceLocation{}, SourceLocation{}, IntExpr);
+    EXPECT_EQ(IntExpr, IgnoreExprNodes(PE, IgnoreParens{}));
+  }
+
+  {
+    IgnoreParens CB{};
+    auto *IntExpr = createIntLiteral(10);
+    ParenExpr *PE =
+        new (Ctx) ParenExpr(SourceLocation{}, SourceLocation{}, IntExpr);
+    EXPECT_EQ(nullptr, IgnoreExprNodes(PE, CB));
+  }
+}

clang/unittests/AST/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   TargetParser
   )
 
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
+  ASTExprTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterObjCTest.cpp
   ASTImporterGenericRedeclTest.cpp
   ASTImporterODRStrategiesTest.cpp
   ASTImporterVisibilityTest.cpp
   ASTTraverserTest.cpp
   ASTTypeTraitsTest.cpp