Page MenuHomePhabricator

[Support] Format provider improvements
Needs ReviewPublic

Authored by vinograd47 on Jan 15 2021, 5:28 AM.

Details

Reviewers
sammccall
zturner
Summary
  • Remove std::forward call for iterator_range iterator de-reference.
  • Use exact type T in has_StreamOperator instead of constant reference.

It fixes formatting usage for some tricky cases, like special ranges,
which de-reference to value type.

It fixes formatting usage with mlir::Operation type,
which is always passed by non-const reference.

Diff Detail

Event Timeline

vinograd47 created this revision.Jan 15 2021, 5:28 AM
vinograd47 requested review of this revision.Jan 15 2021, 5:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 5:28 AM
vinograd47 added a reviewer: Hardcode84.

Why I am here? What's going on?

@Hardcode84 Sorry for confusion, added you by mistake.

Suppress clang-tidy diagnostic about class member name.

It is a common name for compile-time checks implemented via template classes : SomeSheck<T>::value.

Can you add a test for whatever this is fixing?

llvm/include/llvm/Support/FormatProviders.h
405

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.

llvm/include/llvm/Support/FormatVariadicDetails.h
87–88

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.

88

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.

vinograd47 edited the summary of this revision. (Show Details)

Added a test for the case, which this change fixes.

vinograd47 added inline comments.Jan 19 2021, 5:49 AM
llvm/include/llvm/Support/FormatVariadicDetails.h
87–88

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.

88

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.

Rebased and fixes clang-tidy warnings in the new test.

sammccall added inline comments.
llvm/include/llvm/Support/FormatVariadicDetails.h
87–88

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)

88

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.

vinograd47 added inline comments.Jan 20 2021, 2:51 AM
llvm/include/llvm/Support/FormatVariadicDetails.h
88

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.

One more comment.

Even while has_StreamOperator checks only for const correct stream operator,
the formatv implementation still might call non-const version if presented
(due to perfect forwarding).

So, IMHO, either formatv should be stricter or has_StreamOperator should be
relaxed.

@sammccall Could we have some agreement? As I mentioned in the last comment, even while has_StreamOperator checks for const correct version of operator << with streams, formatv implementation can call non-const version if it is available due to perfect forwarding.

Rebased and added a test for range with non-reference return value.

Rebased to restart CI.

Fixed clang-tidy issues in new tests.

flaub added a subscriber: flaub.Mar 10 2021, 12:17 PM