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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to