From e4116700b5e5741ef1b2ca041e8c13af0c71cd89 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Tue, 13 Dec 2022 08:34:50 -0800 Subject: [PATCH] Clean up the interface of io::Printer. This CL kicks any definition longer than three or so lines out of the class body and defines them out of line. The number of private definitions at the head of the class was getting completely out of control. Ideally we would not kick Printer::Sub out, since its API is important, but there's no way to do this while also moving ValueImpl out of the class, which was taking on a whole lot of space. Today I learned that an out-of-line template definition can have multiple template <> declarations, which is necessary to be able to utter the signature of those symbols out of line. PiperOrigin-RevId: 495030857 --- src/google/protobuf/io/printer.cc | 27 +- src/google/protobuf/io/printer.h | 700 ++++++++++++++++-------------- 2 files changed, 390 insertions(+), 337 deletions(-) diff --git a/src/google/protobuf/io/printer.cc b/src/google/protobuf/io/printer.cc index 1320b71cdc..2fc7b6be3c 100644 --- a/src/google/protobuf/io/printer.cc +++ b/src/google/protobuf/io/printer.cc @@ -77,6 +77,30 @@ absl::optional LookupInFrameStack( } } // namespace +struct Printer::Format { + struct Chunk { + // The chunk's text; if this is a variable, it does not include the $...$. + absl::string_view text; + + // Whether or not this is a variable name, i.e., a $...$. + bool is_var; + }; + + struct Line { + // Chunks to emit, split along $ and annotates as to whether it is a + // variable name. + std::vector chunks; + + // The indentation for this chunk. + size_t indent; + }; + + std::vector lines; + + // Whether this is a multiline raw string, according to internal heuristics. + bool is_raw_string = false; +}; + Printer::Format Printer::TokenizeFormat(absl::string_view format_string, const PrintOptions& options) { Format format; @@ -668,7 +692,8 @@ void Printer::PrintImpl(absl::string_view format, "substitution that resolves to callback cannot contain whitespace"); range_start = sink_.bytes_written(); - (*fnc)(); + GOOGLE_ABSL_CHECK((*fnc)()) + << "recursive call encountered while evaluating \"" << var << "\""; range_end = sink_.bytes_written(); } diff --git a/src/google/protobuf/io/printer.h b/src/google/protobuf/io/printer.h index 31deaa5339..4281d14931 100644 --- a/src/google/protobuf/io/printer.h +++ b/src/google/protobuf/io/printer.h @@ -417,159 +417,7 @@ class PROTOBUF_EXPORT Printer { int line() { return 0; } }; - struct AnnotationRecord { - std::vector path; - std::string file_path; - - // AnnotationRecord's constructors are *not* marked as explicit, - // specifically so that it is possible to construct a - // map by writing - // - // {{"foo", my_cool_descriptor}, {"bar", "file.proto"}} - - template < - typename String, - std::enable_if_t::value, - int> = 0> - AnnotationRecord( // NOLINT(google-explicit-constructor) - const String& file_path) - : file_path(file_path) {} - - template ::value, int> = 0> - AnnotationRecord(const Desc* desc) // NOLINT(google-explicit-constructor) - : file_path(desc->file()->name()) { - desc->GetLocationPath(&path); - } - }; - - // Helper type for wrapping a variable substitution expansion result. - template - struct ValueImpl { - private: - template - struct IsSubImpl : std::false_type {}; - template - struct IsSubImpl> : std::true_type {}; - - public: - using StringType = - std::conditional_t; - // These callbacks return false if this is a recursive call. - using Callback = std::function; - using StringOrCallback = absl::variant; - - ValueImpl() = default; - - // This is a template to avoid colliding with the copy constructor below. - template >::value, - int> = 0> - ValueImpl(Value&& value) // NOLINT - : value(ToStringOrCallback(std::forward(value), Rank2{})) { - if (absl::holds_alternative(this->value)) { - consume_after = ";,"; - } - } - - // Copy ctor/assign allow interconversion of the two template parameters. - template - ValueImpl(const ValueImpl& that) { // NOLINT - *this = that; - } - - template - ValueImpl& operator=(const ValueImpl& that) { - // Cast to void* is required, since this and that may potentially be of - // different types (due to the `that_owned` parameter). - if (static_cast(this) == static_cast(&that)) { - return *this; - } - - using ThatStringType = typename ValueImpl::StringType; - - if (auto* str = absl::get_if(&that.value)) { - value = StringType(*str); - } else { - value = absl::get(that.value); - } - - consume_after = that.consume_after; - return *this; - } - - const StringType* AsString() const { - return absl::get_if(&value); - } - - const Callback* AsCallback() const { - return absl::get_if(&value); - } - - StringOrCallback value; - std::string consume_after; - - private: - // go/ranked-overloads - struct Rank0 {}; - struct Rank1 : Rank0 {}; - struct Rank2 : Rank1 {}; - - // Dummy template for delayed instantiation, which is required for the - // static assert below to kick in only when this function is called when it - // shouldn't. - // - // This is done to produce a better error message than the "candidate does - // not match" SFINAE errors. - template ()())> - StringOrCallback ToStringOrCallback(Cb&& cb, Rank2) { - return Callback( - [cb = std::forward(cb), is_called = false]() mutable -> bool { - if (is_called) { - // Catch whether or not this function is being called recursively. - return false; - } - is_called = true; - cb(); - is_called = false; - return true; - }); - } - - // Separate from the AlphaNum overload to avoid copies when taking strings - // by value when in `owned` mode. - StringOrCallback ToStringOrCallback(StringType s, Rank1) { return s; } - - StringOrCallback ToStringOrCallback(const absl::AlphaNum& s, Rank0) { - return StringType(s.Piece()); - } - }; - - using ValueView = ValueImpl; - using Value = ValueImpl; - - // Provide a helper to use heterogeneous lookup when it's available. - template - using void_t = void; - template - struct has_heterogeneous_lookup : std::false_type {}; - template - struct has_heterogeneous_lookup().find( - std::declval()))>> - : std::true_type {}; - - template ::value, int> = 0> - static absl::string_view ToStringKey(absl::string_view x) { - return x; - } - template ::value, int> = 0> - static std::string ToStringKey(absl::string_view x) { - return std::string(x); - } + struct AnnotationRecord; public: static constexpr char kDefaultVariableDelimiter = '$'; @@ -577,42 +425,7 @@ class PROTOBUF_EXPORT Printer { "PROTOC_CODEGEN_TRACE"; // Sink type for constructing substitutions to pass to WithVars() and Emit(). - class Sub { - public: - template - Sub(std::string key, Value&& value) - : key_(std::move(key)), - value_(std::forward(value)), - annotation_(absl::nullopt) {} - - // NOTE: This is an overload rather than taking optional - // with a default argument of nullopt, because we want to pick up - // AnnotationRecord's user-defined conversions. Because going from - // e.g. Descriptor* -> optional requires two user-defined - // conversions, this does not work. - template - Sub(Key&& key, Value&& value, AnnotationRecord annotation) - : key_(std::forward(key)), - value_(std::forward(value)), - annotation_(std::move(annotation)) {} - - Sub Annotate(AnnotationRecord annotation) && { - annotation_ = std::move(annotation); - return std::move(*this); - } - - Sub WithSuffix(std::string sub_suffix) && { - value_.consume_after = std::move(sub_suffix); - return std::move(*this); - } - - private: - friend class Printer; - - std::string key_; - Value value_; - absl::optional annotation_; - }; + class Sub; // Options for controlling how the output of a Printer is formatted. struct Options { @@ -666,39 +479,18 @@ class PROTOBUF_EXPORT Printer { // // Returns an RAII object that pops the lookup frame. template - auto WithVars(const Map* vars) { - var_lookups_.emplace_back( - [vars](absl::string_view var) -> absl::optional { - auto it = vars->find(ToStringKey(var)); - if (it == vars->end()) { - return absl::nullopt; - } - return ValueView(it->second); - }); - return absl::MakeCleanup([this] { var_lookups_.pop_back(); }); - } + auto WithVars(const Map* vars); // Pushes a new variable lookup frame that stores `vars` by value. // - // When writing `WithVars({...})`, this is the overload that will be called, - // and it will synthesize an `absl::flat_hash_map`. - // // Returns an RAII object that pops the lookup frame. template , - std::enable_if_t::value, int> = 0> - auto WithVars(Map&& vars) { - var_lookups_.emplace_back( - [vars = std::forward(vars)]( - absl::string_view var) -> absl::optional { - auto it = vars.find(ToStringKey(var)); - if (it == vars.end()) { - return absl::nullopt; - } - return ValueView(it->second); - }); - return absl::MakeCleanup([this] { var_lookups_.pop_back(); }); - } + typename = std::enable_if_t::value>> + auto WithVars(Map&& vars); + // Pushes a new variable lookup frame that stores `vars` by value. + // + // Returns an RAII object that pops the lookup frame. auto WithVars(std::initializer_list vars); // Looks up a variable set with WithVars(). @@ -712,17 +504,7 @@ class PROTOBUF_EXPORT Printer { // // Returns an RAII object that pops the lookup frame. template - auto WithAnnotations(const Map* vars) { - annotation_lookups_.emplace_back( - [vars](absl::string_view var) -> absl::optional { - auto it = vars->find(ToStringKey(var)); - if (it == vars->end()) { - return absl::nullopt; - } - return AnnotationRecord(it->second); - }); - return absl::MakeCleanup([this] { annotation_lookups_.pop_back(); }); - } + auto WithAnnotations(const Map* vars); // Pushes a new variable lookup frame that stores `vars` by value. // @@ -731,18 +513,7 @@ class PROTOBUF_EXPORT Printer { // // Returns an RAII object that pops the lookup frame. template > - auto WithAnnotations(Map&& vars) { - annotation_lookups_.emplace_back( - [vars = std::forward(vars)]( - absl::string_view var) -> absl::optional { - auto it = vars.find(ToStringKey(var)); - if (it == vars.end()) { - return absl::nullopt; - } - return AnnotationRecord(it->second); - }); - return absl::MakeCleanup([this] { annotation_lookups_.pop_back(); }); - } + auto WithAnnotations(Map&& vars); // Increases the indentation by `indent` spaces; when nullopt, increments // indentation by the configured default spaces_per_indent. @@ -759,9 +530,7 @@ class PROTOBUF_EXPORT Printer { // // `format` MUST be a string constant. void Emit(absl::string_view format, - SourceLocation loc = SourceLocation::current()) { - Emit({}, format, loc); - } + SourceLocation loc = SourceLocation::current()); // Emits formatted source code to the underlying output, injecting // additional variables as a lookup frame for just this call. See the class @@ -788,31 +557,10 @@ class PROTOBUF_EXPORT Printer { // TODO(b/242326974): Deprecate these APIs. template > - void Print(const Map& vars, absl::string_view text) { - PrintOptions opts; - opts.checks_are_debug_only = true; - opts.use_substitution_map = true; - opts.allow_digit_substitutions = false; - - auto pop = WithVars(&vars); - PrintImpl(text, {}, opts); - } + void Print(const Map& vars, absl::string_view text); template - void Print(absl::string_view text, const Args&... args) { - static_assert(sizeof...(args) % 2 == 0, ""); - - // Include an extra arg, since a zero-length array is ill-formed, and - // MSVC complains. - absl::string_view vars[] = {args..., ""}; - absl::flat_hash_map map; - map.reserve(sizeof...(args) / 2); - for (size_t i = 0; i < sizeof...(args); i += 2) { - map.emplace(vars[i], vars[i + 1]); - } - - Print(map, text); - } + void Print(absl::string_view text, const Args&... args); // Link a substitution variable emitted by the last call to Print to the // object described by descriptor. @@ -825,17 +573,9 @@ class PROTOBUF_EXPORT Printer { // the last call to Print to the object described by descriptor. The range // begins at begin_varname's value and ends after the last character of the // value substituted for end_varname. - template + template void Annotate(absl::string_view begin_varname, absl::string_view end_varname, - const SomeDescriptor* descriptor) { - if (options_.annotation_collector == nullptr) { - return; - } - - std::vector path; - descriptor->GetLocationPath(&path); - Annotate(begin_varname, end_varname, descriptor->file()->name(), path); - } + const Desc* descriptor); // Link a substitution variable emitted by the last call to Print to the file // with path file_name. @@ -866,75 +606,41 @@ class PROTOBUF_EXPORT Printer { // compiler::cpp::Formatter instead. template > void FormatInternal(absl::Span args, const Map& vars, - absl::string_view format) { - PrintOptions opts; - opts.use_curly_brace_substitutions = true; - opts.strip_spaces_around_vars = true; - - auto pop = WithVars(&vars); - PrintImpl(format, args, opts); - } + absl::string_view format); private: - // Options for PrintImpl(). - struct PrintOptions { - // The callsite of the public entry-point. Only Emit() sets this. - absl::optional loc; - // If set, Validate() calls will not crash the program. - bool checks_are_debug_only = false; - // If set, the `substitutions_` map will be populated as variables are - // substituted. - bool use_substitution_map = false; - // If set, the ${1$ and $}$ forms will be substituted. These are used for - // a slightly janky annotation-insertion mechanism in FormatInternal, that - // requires that passed-in substitution variables be serialized protos. - bool use_curly_brace_substitutions = false; - // If set, the $n$ forms will be substituted, pulling from the `args` - // argument to PrintImpl(). - bool allow_digit_substitutions = true; - // If set, when a variable substitution with spaces in it, such as $ var$, - // is encountered, the spaces are stripped, so that it is as if it was - // $var$. If $var$ substitutes to a non-empty string, the removed spaces are - // printed around the substituted value. - // - // See the class documentation for more information on this behavior. - bool strip_spaces_around_vars = true; - // If set, leading whitespace will be stripped from the format string to - // determine the "extraneous indentation" that is produced when the format - // string is a C++ raw string. This is used to remove leading spaces from - // a raw string that would otherwise result in erratic indentation in the - // output. - bool strip_raw_string_indentation = false; - // If set, the annotation lookup frames are searched, per the annotation - // semantics of Emit() described in the class documentation. - bool use_annotation_frames = true; - }; - - friend class FormatIterator; + struct PrintOptions; + struct Format; - struct Format { - struct Chunk { - // The chunk's text; if this is a variable, it does not include the $...$. - absl::string_view text; + // Helper type for wrapping a variable substitution expansion result. + template + struct ValueImpl; - // Whether or not this is a variable name, i.e., a $...$. - bool is_var; - }; + using ValueView = ValueImpl; + using Value = ValueImpl; - struct Line { - // Chunks to emit, split along $ and annotates as to whether it is a - // variable name. - std::vector chunks; + // Provide a helper to use heterogeneous lookup when it's available. + template + using Void = void; - // The indentation for this chunk. - size_t indent; - }; + template + struct HasHeteroLookup : std::false_type {}; + template + struct HasHeteroLookup().find( + std::declval()))>> + : std::true_type {}; - std::vector lines; + template ::value>> + static absl::string_view ToStringKey(absl::string_view x) { + return x; + } - // Whether this is a multiline raw string, according to internal heuristics. - bool is_raw_string = false; - }; + template ::value>> + static std::string ToStringKey(absl::string_view x) { + return std::string(x); + } Format TokenizeFormat(absl::string_view format_string, const PrintOptions& options); @@ -1006,6 +712,328 @@ class PROTOBUF_EXPORT Printer { std::vector line_start_variables_; }; +// Options for PrintImpl(). +struct Printer::PrintOptions { + // The callsite of the public entry-point. Only Emit() sets this. + absl::optional loc; + // If set, Validate() calls will not crash the program. + bool checks_are_debug_only = false; + // If set, the `substitutions_` map will be populated as variables are + // substituted. + bool use_substitution_map = false; + // If set, the ${1$ and $}$ forms will be substituted. These are used for + // a slightly janky annotation-insertion mechanism in FormatInternal, that + // requires that passed-in substitution variables be serialized protos. + bool use_curly_brace_substitutions = false; + // If set, the $n$ forms will be substituted, pulling from the `args` + // argument to PrintImpl(). + bool allow_digit_substitutions = true; + // If set, when a variable substitution with spaces in it, such as $ var$, + // is encountered, the spaces are stripped, so that it is as if it was + // $var$. If $var$ substitutes to a non-empty string, the removed spaces are + // printed around the substituted value. + // + // See the class documentation for more information on this behavior. + bool strip_spaces_around_vars = true; + // If set, leading whitespace will be stripped from the format string to + // determine the "extraneous indentation" that is produced when the format + // string is a C++ raw string. This is used to remove leading spaces from + // a raw string that would otherwise result in erratic indentation in the + // output. + bool strip_raw_string_indentation = false; + // If set, the annotation lookup frames are searched, per the annotation + // semantics of Emit() described in the class documentation. + bool use_annotation_frames = true; +}; + +// Helper type for wrapping a variable substitution expansion result. +template +struct Printer::ValueImpl { + private: + template + struct IsSubImpl : std::false_type {}; + template + struct IsSubImpl> : std::true_type {}; + + public: + using StringType = std::conditional_t; + // These callbacks return false if this is a recursive call. + using Callback = std::function; + using StringOrCallback = absl::variant; + + ValueImpl() = default; + + // This is a template to avoid colliding with the copy constructor below. + template >::value>> + ValueImpl(Value&& value) // NOLINT + : value(ToStringOrCallback(std::forward(value), Rank2{})) { + if (absl::holds_alternative(this->value)) { + consume_after = ";,"; + } + } + + // Copy ctor/assign allow interconversion of the two template parameters. + template + ValueImpl(const ValueImpl& that) { // NOLINT + *this = that; + } + + template + ValueImpl& operator=(const ValueImpl& that); + + const StringType* AsString() const { + return absl::get_if(&value); + } + + const Callback* AsCallback() const { return absl::get_if(&value); } + + StringOrCallback value; + std::string consume_after; + + private: + // go/ranked-overloads + struct Rank0 {}; + struct Rank1 : Rank0 {}; + struct Rank2 : Rank1 {}; + + // Dummy template for delayed instantiation, which is required for the + // static assert below to kick in only when this function is called when it + // shouldn't. + // + // This is done to produce a better error message than the "candidate does + // not match" SFINAE errors. + template ()())> + StringOrCallback ToStringOrCallback(Cb&& cb, Rank2); + + // Separate from the AlphaNum overload to avoid copies when taking strings + // by value when in `owned` mode. + StringOrCallback ToStringOrCallback(StringType s, Rank1) { return s; } + + StringOrCallback ToStringOrCallback(const absl::AlphaNum& s, Rank0) { + return StringType(s.Piece()); + } +}; + +template +template +Printer::ValueImpl& Printer::ValueImpl::operator=( + const ValueImpl& that) { + // Cast to void* is required, since this and that may potentially be of + // different types (due to the `that_owned` parameter). + if (static_cast(this) == static_cast(&that)) { + return *this; + } + + using ThatStringType = typename ValueImpl::StringType; + + if (auto* str = absl::get_if(&that.value)) { + value = StringType(*str); + } else { + value = absl::get(that.value); + } + + consume_after = that.consume_after; + return *this; +} + +template +template +auto Printer::ValueImpl::ToStringOrCallback(Cb&& cb, Rank2) + -> StringOrCallback { + return Callback( + [cb = std::forward(cb), is_called = false]() mutable -> bool { + if (is_called) { + // Catch whether or not this function is being called recursively. + return false; + } + is_called = true; + cb(); + is_called = false; + return true; + }); +} + +struct Printer::AnnotationRecord { + std::vector path; + std::string file_path; + + // AnnotationRecord's constructors are *not* marked as explicit, + // specifically so that it is possible to construct a + // map by writing + // + // {{"foo", my_cool_descriptor}, {"bar", "file.proto"}} + + template < + typename String, + std::enable_if_t::value, + int> = 0> + AnnotationRecord( // NOLINT(google-explicit-constructor) + const String& file_path) + : file_path(file_path) {} + + template ::value, int> = 0> + AnnotationRecord(const Desc* desc) // NOLINT(google-explicit-constructor) + : file_path(desc->file()->name()) { + desc->GetLocationPath(&path); + } +}; + +class Printer::Sub { + public: + template + Sub(std::string key, Value&& value) + : key_(std::move(key)), + value_(std::forward(value)), + annotation_(absl::nullopt) {} + + // NOTE: This is an overload rather than taking optional + // with a default argument of nullopt, because we want to pick up + // AnnotationRecord's user-defined conversions. Because going from + // e.g. Descriptor* -> optional requires two user-defined + // conversions, this does not work. + template + Sub(Key&& key, Value&& value, AnnotationRecord annotation) + : key_(std::forward(key)), + value_(std::forward(value)), + annotation_(std::move(annotation)) {} + + Sub Annotate(AnnotationRecord annotation) && { + annotation_ = std::move(annotation); + return std::move(*this); + } + + Sub WithSuffix(std::string sub_suffix) && { + value_.consume_after = std::move(sub_suffix); + return std::move(*this); + } + + private: + friend class Printer; + + std::string key_; + Value value_; + absl::optional annotation_; +}; + +template +auto Printer::WithVars(const Map* vars) { + var_lookups_.emplace_back( + [vars](absl::string_view var) -> absl::optional { + auto it = vars->find(ToStringKey(var)); + if (it == vars->end()) { + return absl::nullopt; + } + return ValueView(it->second); + }); + return absl::MakeCleanup([this] { var_lookups_.pop_back(); }); +} + +template +auto Printer::WithVars(Map&& vars) { + var_lookups_.emplace_back( + [vars = std::forward(vars)]( + absl::string_view var) -> absl::optional { + auto it = vars.find(ToStringKey(var)); + if (it == vars.end()) { + return absl::nullopt; + } + return ValueView(it->second); + }); + return absl::MakeCleanup([this] { var_lookups_.pop_back(); }); +} + +template +auto Printer::WithAnnotations(const Map* vars) { + annotation_lookups_.emplace_back( + [vars](absl::string_view var) -> absl::optional { + auto it = vars->find(ToStringKey(var)); + if (it == vars->end()) { + return absl::nullopt; + } + return AnnotationRecord(it->second); + }); + return absl::MakeCleanup([this] { annotation_lookups_.pop_back(); }); +} + +template +auto Printer::WithAnnotations(Map&& vars) { + annotation_lookups_.emplace_back( + [vars = std::forward(vars)]( + absl::string_view var) -> absl::optional { + auto it = vars.find(ToStringKey(var)); + if (it == vars.end()) { + return absl::nullopt; + } + return AnnotationRecord(it->second); + }); + return absl::MakeCleanup([this] { annotation_lookups_.pop_back(); }); +} + +// In GCC older than GCC 9, this code (which constructs an empty +// std::initializer_list) incorrectly fails to compile if it does not +// follow the definition of the type Sub; this is the only reason it is +// out-of-line. +// +// See https://godbolt.org/z/e4KnM3PE7. +inline void Printer::Emit(absl::string_view format, SourceLocation loc) { + Emit({}, format, loc); +} + +template +void Printer::Print(const Map& vars, absl::string_view text) { + PrintOptions opts; + opts.checks_are_debug_only = true; + opts.use_substitution_map = true; + opts.allow_digit_substitutions = false; + + auto pop = WithVars(&vars); + PrintImpl(text, {}, opts); +} + +template +void Printer::Print(absl::string_view text, const Args&... args) { + static_assert(sizeof...(args) % 2 == 0, ""); + + // Include an extra arg, since a zero-length array is ill-formed, and + // MSVC complains. + absl::string_view vars[] = {args..., ""}; + absl::flat_hash_map map; + map.reserve(sizeof...(args) / 2); + for (size_t i = 0; i < sizeof...(args); i += 2) { + map.emplace(vars[i], vars[i + 1]); + } + + Print(map, text); +} + +template +void Printer::Annotate(absl::string_view begin_varname, + absl::string_view end_varname, const Desc* descriptor) { + if (options_.annotation_collector == nullptr) { + return; + } + + std::vector path; + descriptor->GetLocationPath(&path); + Annotate(begin_varname, end_varname, descriptor->file()->name(), path); +} + +template +void Printer::FormatInternal(absl::Span args, + const Map& vars, absl::string_view format) { + PrintOptions opts; + opts.use_curly_brace_substitutions = true; + opts.strip_spaces_around_vars = true; + + auto pop = WithVars(&vars); + PrintImpl(format, args, opts); +} + inline auto Printer::WithDefs(std::initializer_list vars, bool allow_callbacks) { absl::flat_hash_map var_map;