[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for this, it's a great improvement! I think it's generally correct, but I did have some minor questions.

> Most of these improvements are very small, but I measured a 3% decrease in -O0 object file size for a simple C++ source file using the standard library after this change.

Any chance you'd be interested in submitting this patch to http://llvm-compile-time-tracker.com/ (instructions at http://llvm-compile-time-tracker.com/about.php) so we can have an idea of how compile times are impacted? (It's not critical, but having some more compile time performance measurements would be helpful to validate that this actually saves compile time as expected.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6577-6578
+// FIXME: This should also be in -Wc++23-compat once we have it.
+def warn_use_of_unaddressable_function : Warning<
+  "taking address of non-addressable standard library function">,
+  InGroup<CXX20Compat>;
----------------
Thank you for making this one on by default :-)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:20282
+      if (Context.BuiltinInfo.isInStdNamespace(BuiltinID)) {
+        // Any use of these other than a direct call is ill-formed as of C++20,
+        // because they are not addressable functions. In earlier language
----------------
I wonder how often these get passed around as function objects... e.g.,
```
template <typename Fn>
void func(Fn &&Call);

int main() {
  func(std::move<int>);
}
```
(I don't see evidence that this is a common code pattern, but C++ surprises me often enough that I wonder if this will cause issues.)


================
Comment at: clang/lib/Sema/SemaInit.cpp:8192-8195
+      // We might get back another placeholder expression if we resolved to a
+      // builtin.
+      if (!CurInit.isInvalid())
+        CurInit = S.CheckPlaceholderExpr(CurInit.get());
----------------
Do we need to make this call every time we've called `FixOverloadedFunctionReference()`? I guess I'm not seeing the changes that necessitated this call (or the one below).


================
Comment at: clang/test/SemaCXX/builtin-std-move.cpp:12
+
+  template<typename T> CONSTEXPR T &&move(T &x) {
+    static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
----------------
Formatting looks like it's gone wonky in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345