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>
diff --git a/src/diagnostic/formatter.cc b/src/diagnostic/formatter.cc index abb7232..6257425 100644 --- a/src/diagnostic/formatter.cc +++ b/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 @@ return stream; } -class BasicFormatter : public Formatter { - public: - struct State { - explicit State(Printer* p) : printer(p) {} - ~State() { flush(); } +} // namespace - // set_style() sets the current style to new_style, flushing any pending - // messages to the printer if the style changed. - void set_style(const Style& new_style) { - if (style.color != new_style.color || style.bold != new_style.bold) { - flush(); - style = new_style; - } +/// State holds the internal formatter state for a format() call. +struct Formatter::State { + /// Constructs a State associated with the given printer. + /// @param p the printer to write formatted messages to. + explicit State(Printer* p) : printer(p) {} + ~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. - void flush() { - auto str = stream.str(); - if (str.length() > 0) { - printer->write(str, style); - std::stringstream reset; - stream.swap(reset); - } + /// flush writes any pending messages to the printer, clearing the buffer. + void flush() { + auto str = stream.str(); + if (str.length() > 0) { + printer->write(str, style); + std::stringstream reset; + stream.swap(reset); } + } - // operator<<() queues msg to be written to the printer. - template <typename T> - State& operator<<(const T& msg) { - stream << msg; - return *this; - } + /// operator<< queues msg to be written to the printer. + /// @param msg the value or string to write to the printer + /// @returns this State so that calls can be chained + template <typename T> + State& operator<<(const T& msg) { + stream << msg; + return *this; + } - // newline() queues a newline to be written to the printer. - void newline() { stream << std::endl; } + /// newline queues a newline to be written to the printer. + void newline() { stream << std::endl; } - // repeat() queues the character c to be writen to the printer n times. - 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; + /// repeat queues the character c to be writen to the printer n times. + /// @param c the character to print |n| times + /// @param n the number of times to print character |c| + void repeat(char c, size_t n) { + while (n-- > 0) { + stream << c; } } private: - void format(const Diagnostic& diag, State* state) const { - auto const& src = diag.source; - auto const& rng = src.range; - - 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; + Printer* printer; + diag::Style style; + std::stringstream stream; }; -} // namespace +Formatter::Formatter() {} +Formatter::Formatter(const Style& style) : style_(style) {} -std::unique_ptr<Formatter> Formatter::create(bool print_file, - bool print_severity, - bool print_line) { - return std::make_unique<BasicFormatter>(print_file, print_severity, - print_line); +void Formatter::format(const List& list, Printer* printer) const { + State state{printer}; + + bool first = true; + 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;
diff --git a/src/diagnostic/formatter.h b/src/diagnostic/formatter.h index 43061dd..36d3ef3 100644 --- a/src/diagnostic/formatter.h +++ b/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 -/// string. +/// Formatter are used to print a list of diagnostics messages. class Formatter { public: - /// @returns a basic diagnostic formatter - /// @param print_file include the file path for each diagnostic - /// @param print_severity include the severity for each diagnostic - /// @param print_line include the source line(s) for the diagnostic - static std::unique_ptr<Formatter> create(bool print_file, - bool print_severity, - bool print_line); + /// Style controls the formatter's output style. + struct Style { + /// include the file path for each diagnostic + bool print_file = true; + /// include the severity for each diagnostic + bool print_severity = true; + /// include the source line(s) for the diagnostic + bool print_line = true; + }; - virtual ~Formatter(); + /// Constructor for the formatter using a default style. + Formatter(); - /// format prints the formatted diagnostic list |list| to |printer|. + /// Constructor for the formatter using the custom style. + /// @param style the style used for the formatter. + explicit Formatter(const Style& style); + + ~Formatter(); + /// @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 { - StringPrinter printer; - format(list, &printer); - return printer.str(); - } + std::string format(const List& list) const; + + private: + struct State; + + void format(const Diagnostic& diag, State& state) const; + + Style const style_; }; } // namespace diag
diff --git a/src/diagnostic/formatter_test.cc b/src/diagnostic/formatter_test.cc index ae52b54..69654a2 100644 --- a/src/diagnostic/formatter_test.cc +++ b/src/diagnostic/formatter_test.cc
@@ -44,8 +44,8 @@ }; TEST_F(DiagFormatterTest, Simple) { - auto fmt = Formatter::create(false, false, false); - auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal}); + Formatter fmt{{false, false, false}}; + 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, WithFile) { - auto fmt = Formatter::create(true, false, false); - auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal}); + Formatter fmt{{true, false, false}}; + 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 @@ } TEST_F(DiagFormatterTest, WithSeverity) { - auto fmt = Formatter::create(false, true, false); - auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal}); + Formatter fmt{{false, true, false}}; + 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, WithLine) { - auto fmt = Formatter::create(false, false, true); - auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal}); + Formatter fmt{{false, false, true}}; + 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 @@ } TEST_F(DiagFormatterTest, BasicWithFileSeverityLine) { - auto fmt = Formatter::create(true, true, true); - auto got = fmt->format(List{diag_info, diag_warn, diag_err, diag_fatal}); + Formatter fmt{{true, true, true}}; + 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 @@ Diagnostic multiline{Severity::Warning, Source{Source::Range{{2, 9}, {4, 15}}, &file}, "multiline"}; - - auto fmt = Formatter::create(false, false, true); - auto got = fmt->format(List{multiline}); + Formatter fmt{{false, false, true}}; + auto got = fmt.format(List{multiline}); auto* expect = R"(2:9: multiline the dog says woof ^^^^^^^^^
diff --git a/src/reader/wgsl/parser_impl.h b/src/reader/wgsl/parser_impl.h index 9a902f4..d407048 100644 --- a/src/reader/wgsl/parser_impl.h +++ b/src/reader/wgsl/parser_impl.h
@@ -104,7 +104,8 @@ /// @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
diff --git a/src/reader/wgsl/parser_impl_error_msg_test.cc b/src/reader/wgsl/parser_impl_error_msg_test.cc index e111fd2..2d2d0e2 100644 --- a/src/reader/wgsl/parser_impl_error_msg_test.cc +++ b/src/reader/wgsl/parser_impl_error_msg_test.cc
@@ -23,16 +23,14 @@ class ParserImplErrorTest : public ParserImplTest {}; -#define EXPECT(SOURCE, EXPECTED) \ - do { \ - std::string source = SOURCE; \ - std::string expected = EXPECTED; \ - auto* p = parser(source); \ - EXPECT_EQ(false, p->Parse()); \ - EXPECT_EQ(true, p->diagnostics().contains_errors()); \ - EXPECT_EQ( \ - expected, \ - diag::Formatter::create(true, true, true)->format(p->diagnostics())); \ +#define EXPECT(SOURCE, EXPECTED) \ + do { \ + std::string source = SOURCE; \ + std::string expected = EXPECTED; \ + auto* p = parser(source); \ + EXPECT_EQ(false, p->Parse()); \ + EXPECT_EQ(true, p->diagnostics().contains_errors()); \ + EXPECT_EQ(expected, diag::Formatter().format(p->diagnostics())); \ } while (false) TEST_F(ParserImplErrorTest, AdditiveInvalidExpr) {