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) {
