[PATCH] D34281: [Format] Simplify some of the formatter template glue

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Support/FormatProviders.h:28-33
+#define FORMATTER(Type)                                                        \
+  namespace llvm {                                                             \
+  void format_one_item(Type &&Item, raw_ostream &S, StringRef Options);        \
+  }                                                                            \
+  inline void llvm::format_one_item(Type &&Value, raw_ostream &Stream,         \
+                                    StringRef Style)
----------------
Not sure this benefits much from being a macro, does it?

Calls to these functions look like they're ADL compatible, so there's no need to put them in the llvm namespace - they can go in whatever namespace the Type is in, probably? (doesn't matter too much in the LLVM project - we don't need to protect the llvm namespace, etc, but is perhaps a more generic extensibility mechanism than users having to write function definitions in the library's namespace)


================
Comment at: llvm/include/llvm/Support/FormatProviders.h:394-396
+  if (!Style.empty() && Style.getAsInteger(10, N)) {
+    assert(false && "Style is not a valid integer");
+  }
----------------
Skip braces on one-line if?


================
Comment at: llvm/include/llvm/Support/FormatProviders.h:395
+  if (!Style.empty() && Style.getAsInteger(10, N)) {
+    assert(false && "Style is not a valid integer");
+  }
----------------
Usually assert(false) should be llvm_unreachable, but branch-to-unreachable isn't ideal either, so perhaps:

  if (!Style.empty()) {
    bool Success = Style.getAsInteger(10, N);
    (void)Success;
    assert(Success && "Style is not a valid integer");
  }

Yeah, I know it's a couple of lines longer, which is awkward too :/

I guess if getAsInteger returned Expected<size_t> might be a bit tidier:

  if (!Style.empty())
    N = cantFail(Style.getAsInteger<size_t>(10));


================
Comment at: llvm/include/llvm/Support/FormatProviders.h:404
+/// This follows the same rules as the string formatter.
+FORMATTER(llvm::Twine) { format_one_item(Value.str(), Stream, Style); };
+
----------------
If you can avoid turning a Twine into a std::string it could save some memory allocation/copying/processing.

But, yeah, supporting the length limiting support of the string formatter above would be a bit of work interacting with Twine's print function. (maybe Twine could use a template that takes a functor and is called back on the various kinds of elements it has) - something for another day I guess.


================
Comment at: llvm/include/llvm/Support/FormatVariadicDetails.h:79-83
 template <typename T>
-typename std::enable_if<uses_format_provider<T>::value,
-                        provider_format_adapter<T>>::type
-build_format_adapter(T &&Item) {
-  return provider_format_adapter<T>(std::forward<T>(Item));
+typename std::enable_if<detail::uses_format_member<T>::value, void>::type
+format_one_item(T &&Item, raw_ostream &S, StringRef Options) {
+  Item.format(S, Options);
 }
----------------
Does this support (for format members) pull its weight? Compared to all format extensibility going through a single ADL signature? (the definition of a non-member format function where the member format is written would be almost identical, if defined as an inline friend definition inside the class definition (you have to mention the type name in the signature and name that variable rather than using 'this'))


https://reviews.llvm.org/D34281