[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:88
+  // NOLINTNEXTLINE(readability-identifier-naming)
+  static bool const value = (sizeof(test<T>(nullptr)) == 1);
 };
----------------
sammccall wrote:
> 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.
I can agree with you in general, but not in this particular case. This formatting API is just a some kind of decorator over other APIs (like `<<` operator with stream). It forwards its arguments as is to underlying API. So it only need to check that the `stream << std::forward<T>(arg)` expression is valid C++ code. Such details as const correctness is the details of this underlying code. As long as `stream << std::forward<T>(arg)` is valid expression, I don't see a reason to forbid it in `formatv` function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94769