Make Formatter a non-interface

I had originally created `Formatter` as an interface as I was intending to implement this differently for linux and windows (for terminal coloring).

Color printing is instead implemented by the `Printer` interface / PIMPL classes.

Replace the multi-boolean constructor with a `Style` struct, as this will make life easier when we want to add / remove flags.

Bug: tint:282
Change-Id: I630073ed7a76c023348b66e8a8517b00b2b6a0d2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/31569
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

src/diagnostic/formatter.cc

@@ -18,6 +18,7 @@
 #include <sstream>
 
 #include "src/diagnostic/diagnostic.h"
+#include "src/diagnostic/printer.h"
 
 namespace tint {
 namespace diag {
@@ -57,154 +58,152 @@ std::basic_ostream<CharT, Traits>& operator<<(
   return stream;
 }
 
-class BasicFormatter : public Formatter {
+}  // namespace
- public:
-  struct State {
-    explicit State(Printer* p) : printer(p) {}
-    ~State() { flush(); }
 
-    // set_style() sets the current style to new_style, flushing any pending
+/// State holds the internal formatter state for a format() call.
-    // messages to the printer if the style changed.
+struct Formatter::State {
-    void set_style(const Style& new_style) {
+  /// Constructs a State associated with the given printer.
-      if (style.color != new_style.color || style.bold != new_style.bold) {
+  /// @param p the printer to write formatted messages to.
-        flush();
+  explicit State(Printer* p) : printer(p) {}
-        style = new_style;
+  ~State() { flush(); }
-      }
+
+  /// set_style() sets the current style to new_style, flushing any pending
+  /// messages to the printer if the style changed.
+  /// @param new_style the new style to apply for future written messages.
+  void set_style(const diag::Style& new_style) {
+    if (style.color != new_style.color || style.bold != new_style.bold) {
+      flush();
+      style = new_style;
     }
+  }
 
-    // flush() writes any pending messages to the printer, clearing the buffer.
+  /// flush writes any pending messages to the printer, clearing the buffer.
-    void flush() {
+  void flush() {
-      auto str = stream.str();
+    auto str = stream.str();
-      if (str.length() > 0) {
+    if (str.length() > 0) {
-        printer->write(str, style);
+      printer->write(str, style);
-        std::stringstream reset;
+      std::stringstream reset;
-        stream.swap(reset);
+      stream.swap(reset);
-      }
     }
+  }
 
-    // operator<<() queues msg to be written to the printer.
+  /// operator<< queues msg to be written to the printer.
-    template <typename T>
+  /// @param msg the value or string to write to the printer
-    State& operator<<(const T& msg) {
+  /// @returns this State so that calls can be chained
-      stream << msg;
+  template <typename T>
-      return *this;
+  State& operator<<(const T& msg) {
-    }
+    stream << msg;
+    return *this;
+  }
 
-    // newline() queues a newline to be written to the printer.
+  /// newline queues a newline to be written to the printer.
-    void newline() { stream << std::endl; }
+  void newline() { stream << std::endl; }
 
-    // repeat() queues the character c to be writen to the printer n times.
+  /// repeat queues the character c to be writen to the printer n times.
-    void repeat(char c, size_t n) {
+  /// @param c the character to print |n| times
-      while (n-- > 0) {
+  /// @param n the number of times to print character |c|
-        stream << c;
+  void repeat(char c, size_t n) {
-      }
+    while (n-- > 0) {
-    }
+      stream << c;
-
-   private:
-    Printer* printer;
-    Style style;
-    std::stringstream stream;
-  };
-
-  BasicFormatter(bool print_file, bool print_severity, bool print_line)
-      : print_file_(print_file),
-        print_severity_(print_severity),
-        print_line_(print_line) {}
-
-  void format(const List& list, Printer* printer) const override {
-    State state{printer};
-
-    bool first = true;
-    for (auto diag : list) {
-      state.set_style({});
-      if (!first) {
-        state.newline();
-      }
-      format(diag, &state);
-      first = false;
     }
   }
 
  private:
-  void format(const Diagnostic& diag, State* state) const {
+  Printer* printer;
-    auto const& src = diag.source;
+  diag::Style style;
-    auto const& rng = src.range;
+  std::stringstream stream;
-
-    state->set_style({Color::kDefault, true});
-
-    if (print_file_ && src.file != nullptr && !src.file->path.empty()) {
-      (*state) << src.file->path;
-      if (rng.begin.line > 0) {
-        (*state) << ":" << rng.begin;
-      }
-    } else {
-      (*state) << rng.begin;
-    }
-    if (print_severity_) {
-      switch (diag.severity) {
-        case Severity::Warning:
-          state->set_style({Color::kYellow, true});
-          break;
-        case Severity::Error:
-        case Severity::Fatal:
-          state->set_style({Color::kRed, true});
-          break;
-        default:
-          break;
-      }
-      (*state) << " " << diag.severity;
-    }
-
-    state->set_style({Color::kDefault, true});
-    (*state) << ": " << diag.message;
-
-    if (print_line_ && src.file != nullptr && rng.begin.line > 0) {
-      state->newline();
-      state->set_style({Color::kDefault, false});
-
-      for (size_t line = rng.begin.line; line <= rng.end.line; line++) {
-        if (line < src.file->lines.size() + 1) {
-          auto len = src.file->lines[line - 1].size();
-
-          (*state) << src.file->lines[line - 1];
-          state->newline();
-          state->set_style({Color::kCyan, false});
-
-          if (line == rng.begin.line && line == rng.end.line) {
-            // Single line
-            state->repeat(' ', rng.begin.column - 1);
-            state->repeat(
-                '^', std::max<size_t>(rng.end.column - rng.begin.column, 1));
-          } else if (line == rng.begin.line) {
-            // Start of multi-line
-            state->repeat(' ', rng.begin.column - 1);
-            state->repeat('^', len - (rng.begin.column - 1));
-          } else if (line == rng.end.line) {
-            // End of multi-line
-            state->repeat('^', rng.end.column - 1);
-          } else {
-            // Middle of multi-line
-            state->repeat('^', len);
-          }
-          state->newline();
-        }
-      }
-
-      state->set_style({});
-    }
-  }
-
-  const bool print_file_ = false;
-  const bool print_severity_ = false;
-  const bool print_line_ = false;
 };
 
-}  // namespace
+Formatter::Formatter() {}
+Formatter::Formatter(const Style& style) : style_(style) {}
 
-std::unique_ptr<Formatter> Formatter::create(bool print_file,
+void Formatter::format(const List& list, Printer* printer) const {
-                                             bool print_severity,
+  State state{printer};
-                                             bool print_line) {
+
-  return std::make_unique<BasicFormatter>(print_file, print_severity,
+  bool first = true;
-                                          print_line);
+  for (auto diag : list) {
+    state.set_style({});
+    if (!first) {
+      state.newline();
+    }
+    format(diag, state);
+    first = false;
+  }
+}
+
+void Formatter::format(const Diagnostic& diag, State& state) const {
+  auto const& src = diag.source;
+  auto const& rng = src.range;
+
+  state.set_style({Color::kDefault, true});
+
+  if (style_.print_file && src.file != nullptr && !src.file->path.empty()) {
+    state << src.file->path;
+    if (rng.begin.line > 0) {
+      state << ":" << rng.begin;
+    }
+  } else {
+    state << rng.begin;
+  }
+  if (style_.print_severity) {
+    switch (diag.severity) {
+      case Severity::Warning:
+        state.set_style({Color::kYellow, true});
+        break;
+      case Severity::Error:
+      case Severity::Fatal:
+        state.set_style({Color::kRed, true});
+        break;
+      default:
+        break;
+    }
+    state << " " << diag.severity;
+  }
+
+  state.set_style({Color::kDefault, true});
+  state << ": " << diag.message;
+
+  if (style_.print_line && src.file != nullptr && rng.begin.line > 0) {
+    state.newline();
+    state.set_style({Color::kDefault, false});
+
+    for (size_t line = rng.begin.line; line <= rng.end.line; line++) {
+      if (line < src.file->lines.size() + 1) {
+        auto len = src.file->lines[line - 1].size();
+
+        state << src.file->lines[line - 1];
+
+        state.newline();
+        state.set_style({Color::kCyan, false});
+
+        if (line == rng.begin.line && line == rng.end.line) {
+          // Single line
+          state.repeat(' ', rng.begin.column - 1);
+          state.repeat('^',
+                       std::max<size_t>(rng.end.column - rng.begin.column, 1));
+        } else if (line == rng.begin.line) {
+          // Start of multi-line
+          state.repeat(' ', rng.begin.column - 1);
+          state.repeat('^', len - (rng.begin.column - 1));
+        } else if (line == rng.end.line) {
+          // End of multi-line
+          state.repeat('^', rng.end.column - 1);
+        } else {
+          // Middle of multi-line
+          state.repeat('^', len);
+        }
+        state.newline();
+      }
+    }
+
+    state.set_style({});
+  }
+}
+
+std::string Formatter::format(const List& list) const {
+  StringPrinter printer;
+  format(list, &printer);
+  return printer.str();
 }
 
 Formatter::~Formatter() = default;

src/diagnostic/formatter.h

@@ -18,39 +18,49 @@
 #include <memory>
 #include <string>
 
-#include "src/diagnostic/printer.h"
-
 namespace tint {
 namespace diag {
 
+class Diagnostic;
 class List;
+class Printer;
 
-/// Formatters are used to format a list of diagnostics into a human readable
+/// Formatter are used to print a list of diagnostics messages.
-/// string.
 class Formatter {
  public:
-  /// @returns a basic diagnostic formatter
+  /// Style controls the formatter's output style.
-  /// @param print_file include the file path for each diagnostic
+  struct Style {
-  /// @param print_severity include the severity for each diagnostic
+    /// include the file path for each diagnostic
-  /// @param print_line include the source line(s) for the diagnostic
+    bool print_file = true;
-  static std::unique_ptr<Formatter> create(bool print_file,
+    /// include the severity for each diagnostic
-                                           bool print_severity,
+    bool print_severity = true;
-                                           bool print_line);
+    /// include the source line(s) for the diagnostic
+    bool print_line = true;
+  };
 
-  virtual ~Formatter();
+  /// Constructor for the formatter using a default style.
+  Formatter();
+
+  /// Constructor for the formatter using the custom style.
+  /// @param style the style used for the formatter.
+  explicit Formatter(const Style& style);
+
+  ~Formatter();
 
-  /// format prints the formatted diagnostic list |list| to |printer|.
   /// @param list the list of diagnostic messages to format
   /// @param printer the printer used to display the formatted diagnostics
-  virtual void format(const List& list, Printer* printer) const = 0;
+  void format(const List& list, Printer* printer) const;
 
   /// @return the list of diagnostics |list| formatted to a string.
   /// @param list the list of diagnostic messages to format
-  inline std::string format(const List& list) const {
+  std::string format(const List& list) const;
-    StringPrinter printer;
+
-    format(list, &printer);
+ private:
-    return printer.str();
+  struct State;
-  }
+
+  void format(const Diagnostic& diag, State& state) const;
+
+  Style const style_;
 };
 
 }  // namespace diag

src/diagnostic/formatter_test.cc

@@ -44,8 +44,8 @@ class DiagFormatterTest : public testing::Test {
 };
 
 TEST_F(DiagFormatterTest, Simple) {
-  auto fmt = Formatter::create(false, false, false);
+  Formatter fmt{{false, false, false}};
-  auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal});
+  auto got = fmt.format(List{diag_info, diag_warn, diag_err, diag_fatal});
   auto* expect = R"(1:14: purr
 2:14: grrr
 3:16: hiss
@@ -54,8 +54,8 @@ TEST_F(DiagFormatterTest, Simple) {
 }
 
 TEST_F(DiagFormatterTest, WithFile) {
-  auto fmt = Formatter::create(true, false, false);
+  Formatter fmt{{true, false, false}};
-  auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal});
+  auto got = fmt.format(List{diag_info, diag_warn, diag_err, diag_fatal});
   auto* expect = R"(file.name:1:14: purr
 file.name:2:14: grrr
 file.name:3:16: hiss
@@ -64,8 +64,8 @@ file.name:4:16: nothing)";
 }
 
 TEST_F(DiagFormatterTest, WithSeverity) {
-  auto fmt = Formatter::create(false, true, false);
+  Formatter fmt{{false, true, false}};
-  auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal});
+  auto got = fmt.format(List{diag_info, diag_warn, diag_err, diag_fatal});
   auto* expect = R"(1:14 info: purr
 2:14 warning: grrr
 3:16 error: hiss
@@ -74,8 +74,8 @@ TEST_F(DiagFormatterTest, WithSeverity) {
 }
 
 TEST_F(DiagFormatterTest, WithLine) {
-  auto fmt = Formatter::create(false, false, true);
+  Formatter fmt{{false, false, true}};
-  auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal});
+  auto got = fmt.format(List{diag_info, diag_warn, diag_err, diag_fatal});
   auto* expect = R"(1:14: purr
 the cat says meow
              ^
@@ -96,8 +96,8 @@ the snail says ???
 }
 
 TEST_F(DiagFormatterTest, BasicWithFileSeverityLine) {
-  auto fmt = Formatter::create(true, true, true);
+  Formatter fmt{{true, true, true}};
-  auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal});
+  auto got = fmt.format(List{diag_info, diag_warn, diag_err, diag_fatal});
   auto* expect = R"(file.name:1:14 info: purr
 the cat says meow
              ^
@@ -121,9 +121,8 @@ TEST_F(DiagFormatterTest, BasicWithMultiLine) {
   Diagnostic multiline{Severity::Warning,
                        Source{Source::Range{{2, 9}, {4, 15}}, &file},
                        "multiline"};
-
+  Formatter fmt{{false, false, true}};
-  auto fmt = Formatter::create(false, false, true);
+  auto got = fmt.format(List{multiline});
-  auto got = fmt->format(List{multiline});
   auto* expect = R"(2:9: multiline
 the dog says woof
         ^^^^^^^^^

src/reader/wgsl/parser_impl.h

@@ -104,7 +104,8 @@ class ParserImpl {
 
   /// @returns the parser error string
   std::string error() const {
-    return diag::Formatter::create(false, false, false)->format(diags_);
+    diag::Formatter formatter{{false, false, false}};
+    return formatter.format(diags_);
   }
 
   /// @returns the diagnostic messages

src/reader/wgsl/parser_impl_error_msg_test.cc

@@ -23,16 +23,14 @@ namespace {
 
 class ParserImplErrorTest : public ParserImplTest {};
 
-#define EXPECT(SOURCE, EXPECTED)                                              \
+#define EXPECT(SOURCE, EXPECTED)                                     \
-  do {                                                                        \
+  do {                                                               \
-    std::string source = SOURCE;                                              \
+    std::string source = SOURCE;                                     \
-    std::string expected = EXPECTED;                                          \
+    std::string expected = EXPECTED;                                 \
-    auto* p = parser(source);                                                 \
+    auto* p = parser(source);                                        \
-    EXPECT_EQ(false, p->Parse());                                             \
+    EXPECT_EQ(false, p->Parse());                                    \
-    EXPECT_EQ(true, p->diagnostics().contains_errors());                      \
+    EXPECT_EQ(true, p->diagnostics().contains_errors());             \
-    EXPECT_EQ(                                                                \
+    EXPECT_EQ(expected, diag::Formatter().format(p->diagnostics())); \
-        expected,                                                             \
-        diag::Formatter::create(true, true, true)->format(p->diagnostics())); \
   } while (false)
 
 TEST_F(ParserImplErrorTest, AdditiveInvalidExpr) {