From 8367f4c9e51f644b90ae82f1bd2041179f76c91e Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Tue, 26 Mar 2024 15:39:54 -0700 Subject: [PATCH] Remove `HintFmt::operator%` Change-Id: Ibcf1a7848b4b18ec9b0807628ff229079ae7a0fe --- src/build-remote/build-remote.cc | 45 ++++---- src/libexpr/print.cc | 2 +- src/libexpr/print.hh | 2 +- src/libutil/fmt.hh | 183 +++++++++++++++---------------- src/libutil/logging.hh | 4 +- tests/unit/libutil/fmt.cc | 23 ++++ 6 files changed, 138 insertions(+), 121 deletions(-) create mode 100644 tests/unit/libutil/fmt.cc diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index e2ada70cd..f7a159829 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -185,16 +185,6 @@ static int main_build_remote(int argc, char * * argv) std::cerr << "# postpone\n"; else { - // build the hint template. - std::string errorText = - "Failed to find a machine for remote build!\n" - "derivation: %s\nrequired (system, features): (%s, [%s])"; - errorText += "\n%s available machines:"; - errorText += "\n(systems, maxjobs, supportedFeatures, mandatoryFeatures)"; - - for (unsigned int i = 0; i < machines.size(); ++i) - errorText += "\n([%s], %s, [%s], [%s])"; - // add the template values. std::string drvstr; if (drvPath.has_value()) @@ -202,19 +192,30 @@ static int main_build_remote(int argc, char * * argv) else drvstr = ""; - auto error = HintFmt::fromFormatString(errorText); - error - % drvstr - % neededSystem - % concatStringsSep(", ", requiredFeatures) - % machines.size(); + std::string machinesFormatted; - for (auto & m : machines) - error - % concatStringsSep(", ", m.systemTypes) - % m.maxJobs - % concatStringsSep(", ", m.supportedFeatures) - % concatStringsSep(", ", m.mandatoryFeatures); + for (auto & m : machines) { + machinesFormatted += HintFmt( + "\n([%s], %s, [%s], [%s])", + concatStringsSep(", ", m.systemTypes), + m.maxJobs, + concatStringsSep(", ", m.supportedFeatures), + concatStringsSep(", ", m.mandatoryFeatures) + ).str(); + } + + auto error = HintFmt( + "Failed to find a machine for remote build!\n" + "derivation: %s\n" + "required (system, features): (%s, [%s])\n" + "%s available machines:\n" + "(systems, maxjobs, supportedFeatures, mandatoryFeatures)%s", + drvstr, + neededSystem, + concatStringsSep(", ", requiredFeatures), + machines.size(), + Uncolored(machinesFormatted) + ); printMsg(couldBuildLocally ? lvlChatty : lvlWarn, error.str()); diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 71ab1edbf..1033c0059 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -607,7 +607,7 @@ std::ostream & operator<<(std::ostream & output, const ValuePrinter & printer) } template<> -HintFmt & HintFmt::operator%(const ValuePrinter & value) +fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & value) { fmt % value; return *this; diff --git a/src/libexpr/print.hh b/src/libexpr/print.hh index 7ddda81b8..94cb11ca7 100644 --- a/src/libexpr/print.hh +++ b/src/libexpr/print.hh @@ -86,6 +86,6 @@ std::ostream & operator<<(std::ostream & output, const ValuePrinter & printer); * magenta. */ template<> -HintFmt & HintFmt::operator%(const ValuePrinter & value); +fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & value); } diff --git a/src/libutil/fmt.hh b/src/libutil/fmt.hh index 84a3e3e11..c68591b72 100644 --- a/src/libutil/fmt.hh +++ b/src/libutil/fmt.hh @@ -5,43 +5,94 @@ #include #include "ansicolor.hh" - namespace nix { -namespace { /** - * A helper for writing `boost::format` expressions. + * Values wrapped in this struct are printed in magenta. * - * These are equivalent: - * - * ``` - * formatHelper(formatter, a_0, ..., a_n) - * formatter % a_0 % ... % a_n - * ``` - * - * With a single argument, `formatHelper(s)` is a no-op. + * By default, arguments to `HintFmt` are printed in magenta. To avoid this, + * either wrap the argument in `Uncolored` or add a specialization of + * `HintFmt::operator%`. */ -template -inline void formatHelper(F & f) -{ } - -template -inline void formatHelper(F & f, const T & x, const Args & ... args) +template +struct Magenta { - // Interpolate one argument and then recurse. - formatHelper(f % x, args...); + Magenta(const T & s) : value(s) {} + const T & value; +}; + +template +std::ostream & operator<<(std::ostream & out, const Magenta & y) +{ + return out << ANSI_MAGENTA << y.value << ANSI_NORMAL; } +/** + * Values wrapped in this class are printed without coloring. + * + * By default, arguments to `HintFmt` are printed in magenta (see `Magenta`). + */ +template +struct Uncolored +{ + Uncolored(const T & s) : value(s) {} + const T & value; +}; + +template +std::ostream & operator<<(std::ostream & out, const Uncolored & y) +{ + return out << ANSI_NORMAL << y.value; +} + +namespace fmt_internal { + /** * Set the correct exceptions for `fmt`. */ -void setExceptions(boost::format & fmt) +inline void setExceptions(boost::format & fmt) { fmt.exceptions( - boost::io::all_error_bits ^ - boost::io::too_many_args_bit ^ - boost::io::too_few_args_bit); + boost::io::all_error_bits ^ boost::io::too_many_args_bit ^ boost::io::too_few_args_bit + ); } + +/** + * Helper class for `HintFmt` that supports the evil `operator%`. + * + * See: https://git.lix.systems/lix-project/lix/issues/178 + */ +struct HintFmt +{ + boost::format fmt; + + template + HintFmt(boost::format && fmt, const Args &... args) : fmt(std::move(fmt)) + { + setExceptions(fmt); + (*this % ... % args); + } + + template + HintFmt & operator%(const T & value) + { + fmt % Magenta(value); + return *this; + } + + template + HintFmt & operator%(const Uncolored & value) + { + fmt % value.value; + return *this; + } + + boost::format into_format() + { + return std::move(fmt); + } +}; + } /** @@ -77,52 +128,14 @@ inline std::string fmt(const char * s) } template -inline std::string fmt(const std::string & fs, const Args & ... args) +inline std::string fmt(const std::string & fs, const Args &... args) { boost::format f(fs); - setExceptions(f); - formatHelper(f, args...); + fmt_internal::setExceptions(f); + (f % ... % args); return f.str(); } -/** - * Values wrapped in this struct are printed in magenta. - * - * By default, arguments to `HintFmt` are printed in magenta. To avoid this, - * either wrap the argument in `Uncolored` or add a specialization of - * `HintFmt::operator%`. - */ -template -struct Magenta -{ - Magenta(const T &s) : value(s) {} - const T & value; -}; - -template -std::ostream & operator<<(std::ostream & out, const Magenta & y) -{ - return out << ANSI_WARNING << y.value << ANSI_NORMAL; -} - -/** - * Values wrapped in this class are printed without coloring. - * - * By default, arguments to `HintFmt` are printed in magenta (see `Magenta`). - */ -template -struct Uncolored -{ - Uncolored(const T & s) : value(s) {} - const T & value; -}; - -template -std::ostream & operator<<(std::ostream & out, const Uncolored & y) -{ - return out << ANSI_NORMAL << y.value; -} - /** * A wrapper around `boost::format` which colors interpolated arguments in * magenta by default. @@ -137,46 +150,28 @@ public: * Format the given string literally, without interpolating format * placeholders. */ - HintFmt(const std::string & literal) - : HintFmt("%s", Uncolored(literal)) - { } - - static HintFmt fromFormatString(const std::string & format) { - return HintFmt(boost::format(format)); - } + HintFmt(const std::string & literal) : HintFmt("%s", Uncolored(literal)) {} /** * Interpolate the given arguments into the format string. */ template - HintFmt(const std::string & format, const Args & ... args) + HintFmt(const std::string & format, const Args &... args) : HintFmt(boost::format(format), args...) - { } + { + } - HintFmt(const HintFmt & hf) - : fmt(hf.fmt) - { } + HintFmt(const HintFmt & hf) : fmt(hf.fmt) {} template - HintFmt(boost::format && fmt, const Args & ... args) - : fmt(std::move(fmt)) + HintFmt(boost::format && fmt, const Args &... args) + : fmt(fmt_internal::HintFmt(std::move(fmt), args...).into_format()) { - setExceptions(fmt); - formatHelper(*this, args...); - } - - template - HintFmt & operator%(const T & value) - { - fmt % Magenta(value); - return *this; - } - - template - HintFmt & operator%(const Uncolored & value) - { - fmt % value.value; - return *this; + if (this->fmt.remaining_args() != 0) { + throw boost::io::too_few_args( + this->fmt.bound_args() + this->fmt.fed_args(), this->fmt.expected_args() + ); + } } std::string str() const diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 7a6341d70..298d66060 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -242,9 +242,7 @@ extern Verbosity verbosity; template inline void warn(const std::string & fs, const Args & ... args) { - boost::format f(fs); - formatHelper(f, args...); - logger->warn(f.str()); + logger->warn(HintFmt(fs, args...).str()); } #define warnOnce(haveWarned, args...) \ diff --git a/tests/unit/libutil/fmt.cc b/tests/unit/libutil/fmt.cc new file mode 100644 index 000000000..383a688d3 --- /dev/null +++ b/tests/unit/libutil/fmt.cc @@ -0,0 +1,23 @@ +#include "fmt.hh" +#include "ansicolor.hh" + +#include + +namespace nix { + +TEST(HintFmt, arg_count) +{ + // Single arg is treated as a literal string. + ASSERT_EQ(HintFmt("%s").str(), "%s"); + + // Other strings format as expected: + ASSERT_EQ(HintFmt("%s", 1).str(), ANSI_MAGENTA "1" ANSI_NORMAL); + ASSERT_EQ(HintFmt("%1%", "hello").str(), ANSI_MAGENTA "hello" ANSI_NORMAL); + + // Argument counts are detected at construction. + ASSERT_THROW(HintFmt("%s %s", 1), boost::io::too_few_args); + + ASSERT_THROW(HintFmt("%s", 1, 2), boost::io::too_many_args); +} + +}