[PATCH] D94769: [Support] Format provider improvements

[Vladislav Vinogradov via Phabricator via llvm-commits]

vinograd47 added inline comments.


================
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);
----------------
sammccall wrote:
> 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.
Without the suppression CI fails on the affected line. I believe it runs checks only on modified code and it started to complain about this case, since I've touched it.

If I remove the suppression comment, CI will fail.


================
Comment at: llvm/include/llvm/Support/FormatVariadicDetails.h:88
+  // NOLINTNEXTLINE(readability-identifier-naming)
+  static bool const value = (sizeof(test<T>(nullptr)) == 1);
 };
----------------
sammccall wrote:
> 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.
Added information and test for the second change (`has_StreamOperator`). It fixes another issue - stream operator, which accepts object by non-const reference.

The issue comes from MLIR project, which use non-const API for Operation class. It is intentional design choice (https://mlir.llvm.org/docs/Rationale/UsageOfConst/) and affects all methods of the class including printing.

While such API is not very common for C++, I think the format utilities should not restrict and refuse such cases. That's why I've made `has_StreamOperator` check more general.

Added an unit test for such scenario.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94769