[PATCH] D132747: [llvm][ADT] Overload output stream operator `<<` for `StringMapEntry` and `StringMap`.

[Dmitri Gribenko via Phabricator via llvm-commits]

gribozavr2 added a comment.

Please clarify in the CL description that the more useful error messages will appear in unit test failures, for example when UnorderedElementsAre() is used with StringMap.



================
Comment at: llvm/include/llvm/ADT/StringMap.h:502
+  }
+  llvm::sort(Lines.begin(), Lines.end());
+  Lines.insert(Lines.begin(), "{");
----------------



================
Comment at: llvm/include/llvm/ADT/StringMapEntry.h:150
+template <typename T, typename = std::void_t<>>
+struct CanStream : std::false_type {};
+
----------------
WDYT about "CanOutputToOstream"?


================
Comment at: llvm/include/llvm/ADT/StringMapEntry.h:150
+template <typename T, typename = std::void_t<>>
+struct CanStream : std::false_type {};
+
----------------
gribozavr2 wrote:
> WDYT about "CanOutputToOstream"?
Similar to hiding implementation details in DenseMap.h and in other headers, I'd suggest to put this type trait inside `namespace detail {}`.


================
Comment at: llvm/include/llvm/ADT/StringMapEntry.h:163
+  } else {
+    OS << "some value ...";
+  }
----------------



================
Comment at: llvm/include/llvm/ADT/StringMapEntry.h:166
+  return OS << "}";
+}
+
----------------
I wonder if we should put these operators into llvm/Testing/ADT, since these operators are only supposed to be used from tests. For production code, LLVM strongly prefers llvm::raw_ostream, a much more light-weight type than std::ostream.

In fact, this is the first `operator<<(std::ostream&, <anything>)` overload in llvm/ADT. Printable types (like APInt) are printable into raw_ostream.

Another option is to provide `operator<<(llvm::raw_ostream&)` in llvm/ADT, and wrap them in `operator<<(std::ostream&)` in llvm/Testing/ADT. The llvm/ADT version won't have the SFINAE magic, but the llvm/Testing/ADT one will.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132747