[Support] Format provider improvements

[vinograd47]

Summary

Remove std::forward call for iterator_range iterator de-reference.
It fixes formatting usage for some tricky cases, like special ranges,
which de-reference to value type.

[Hardcode84]

Why I am here? What's going on?

[vinograd47]

@Hardcode84 Sorry for confusion, added you by mistake.

[vinograd47]

Suppress clang-tidy diagnostic about class member name.

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

[sammccall]

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 ↗

(On Diff #317215)

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 ↗

(On Diff #317215)

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]

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

[vinograd47]

llvm/include/llvm/Support/FormatVariadicDetails.h

87 ↗

(On Diff #317215)

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 ↗

(On Diff #317215)

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.

[vinograd47]

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

[sammccall]

llvm/include/llvm/Support/FormatVariadicDetails.h

87 ↗

(On Diff #317215)

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 ↗

(On Diff #317215)

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]

llvm/include/llvm/Support/FormatVariadicDetails.h

88 ↗

(On Diff #317215)

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.

[vinograd47]

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.

[vinograd47]

@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.

[vinograd47]

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

[vinograd47]

Rebased to restart CI.

[vinograd47]

Fixed clang-tidy issues in new tests.

[vinograd47]

@sammccall @zturner ping

[vinograd47]

Rebased

[vinograd47]

@sammccall @zturner

I've excluded questionable part (non-const references support) from the patch.
Could you please take a look one more time?

[vinograd47]

Rebased to re-run pre-commit CI

[vinograd47]

@sammccall @zturner Gentle remainder :) Could you please take a look one more time to this patch? I've excluded questionable part from it.

[sammccall]

Sorry for the delay :-(

llvm/include/llvm/Support/FormatProviders.h

 /// to format the value type.
 ///
 /// Note that attempting to format an `iterator_range<T>` where no format
 /// provider can be found for T will result in a compile error.
 ///
 
 template <typename IterT> class format_provider<llvm::iterator_range<IterT>> {
   using value = typename std::iterator_traits<IterT>::value_type;
-  using reference = typename std::iterator_traits<IterT>::reference;
 
   static StringRef consumeOneOption(StringRef &Style, char Indicator,
                                     StringRef Default) {
     if (Style.empty())
       return Default;
     if (Style.front() != Indicator)
       return Default;
     Style = Style.drop_front();
public:
   static void format(const llvm::iterator_range<IterT> &V,
                      llvm::raw_ostream &Stream, StringRef Style) {
     StringRef Sep;
     StringRef ArgStyle;
     std::tie(Sep, ArgStyle) = parseOptions(Style);
     auto Begin = V.begin();
     auto End = V.end();
     if (Begin != End) {
-      auto Adapter =
-          detail::build_format_adapter(std::forward<reference>(*Begin));
+      auto Adapter = detail::build_format_adapter(*Begin);
       Adapter.format(Stream, ArgStyle);
       ++Begin;
     }
     while (Begin != End) {
       Stream << Sep;
-      auto Adapter =
-          detail::build_format_adapter(std::forward<reference>(*Begin));
+      auto Adapter = detail::build_format_adapter(*Begin);
       Adapter.format(Stream, ArgStyle);
       ++Begin;
     }
   }
 };
 }
 
 #endif

llvm/unittests/Support/FormatVariadicTest.cpp

 
 TEST(FormatVariadicTest, FormatError) {
   auto E1 = make_error<StringError>("X", inconvertibleErrorCode());
   EXPECT_EQ("X", formatv("{0}", E1).str());
   EXPECT_TRUE(E1.isA<StringError>()); // not consumed
   EXPECT_EQ("X", formatv("{0}", fmt_consume(std::move(E1))).str());
   EXPECT_FALSE(E1.isA<StringError>()); // consumed
 }
+
+TEST(FormatVariadicTest, FormatFilterRange) {
+  std::vector<int> Vec{0, 1, 2};
+  auto Range = map_range(Vec, [](int V) { return V + 1; });
+  EXPECT_EQ("1, 2, 3", formatv("{0}", Range).str());
+}
+
+namespace {
+
+class IntegerValuesRange final
+    : public indexed_accessor_range<IntegerValuesRange, NoneType, int, int *,
+                                    int> {
+public:
+  using indexed_accessor_range<IntegerValuesRange, NoneType, int, int *,
+                               int>::indexed_accessor_range;
+
+  static int dereference(const NoneType &, ptrdiff_t Index) {
+    return static_cast<int>(Index);
+  }
+};
+
+TEST(FormatVariadicTest, FormatRangeNonRef) {
+  IntegerValuesRange Range(None, 0, 3);
+  EXPECT_EQ("0, 1, 2",
+            formatv("{0}", make_range(Range.begin(), Range.end())).str());
+}
+
+} // namespace