[PATCH] D94769: [Support] Format provider improvements

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

Can you add a test for whatever this is fixing?



================
Comment at: llvm/include/llvm/Support/FormatProviders.h:405
     if (Begin != End) {
-      auto Adapter =
-          detail::build_format_adapter(std::forward<reference>(*Begin));
+      auto Adapter = detail::build_format_adapter(*Begin);
       Adapter.format(Stream, ArgStyle);
----------------
As you say this only affects cases where operator* returns something other than `reference`.
I can't see this breaking anything without breaking the build, so if all tests pass I think we're good.


================
Comment at: llvm/include/llvm/Support/FormatVariadicDetails.h:87
 
-  static bool const value = (sizeof(test<ConstRefT>(nullptr)) == 1);
+  // NOLINTNEXTLINE(readability-identifier-naming)
+  static bool const value = (sizeof(test<T>(nullptr)) == 1);
----------------
nit: I don't think it's worth adding suppression comments for the naming lint, since there are *so* many places using the "wrong" style in LLVM.


================
Comment at: llvm/include/llvm/Support/FormatVariadicDetails.h:88
+  // NOLINTNEXTLINE(readability-identifier-naming)
+  static bool const value = (sizeof(test<T>(nullptr)) == 1);
 };
----------------
why this change?
this seems like it should only affect:
 - cases where operator<< has a weird signature (like non-const reference, or by value but not copyable)
 - weird cases due to conversions affecting deduction (but seems as likely to break things as fix them)
All else equal, I think it's preferable to ensure operator<< has the expected signature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94769