[PATCH] D94769: [Support] Format provider improvements
Sam McCall via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 20 02:08:40 PST 2021
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
More information about the llvm-commits
mailing list