[PATCH] D94769: [Support] Format provider improvements

[Sam McCall via Phabricator via llvm-commits]

sammccall added subscribers: kuhnel, goncharov.
sammccall 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);
----------------
vinograd47 wrote:
> 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.
> If I remove the suppression comment, CI will fail.

AFAIK, the clang-tidy linter is not intended to be considered blocking, and there hasn't been a consensus to add lint suppression comments in LLVM.

(@kuhnel @goncharov FYI that this is being seen as a reason to add these comments to changes in files that already do not follow the naming style)


================
Comment at: llvm/include/llvm/Support/FormatVariadicDetails.h:88
+  // NOLINTNEXTLINE(readability-identifier-naming)
+  static bool const value = (sizeof(test<T>(nullptr)) == 1);
 };
----------------
vinograd47 wrote:
> 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.
> I think the format utilities should not restrict and refuse such cases

I disagree - LLVM in general (including Support) is const-correct. If MLIR chooses not to be const-correct, that code should expect to have to const_cast at API boundaries with common code.

A non-const operator<< is unusual, and can have meaning beyond "I don't care about const". e.g. there are at least two operator<< in LLVM that actually mutate their RHS argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94769