Remove HintFmt::operator%

Change-Id: Ibcf1a7848b4b18ec9b0807628ff229079ae7a0fe

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 = "<unknown>";
 
-                        auto error = HintFmt::fromFormatString(errorText);
+                        std::string machinesFormatted;
-                        error
-                            % drvstr
-                            % neededSystem
-                            % concatStringsSep<StringSet>(", ", requiredFeatures)
-                            % machines.size();
 
-                        for (auto & m : machines)
+                        for (auto & m : machines) {
-                            error
+                            machinesFormatted += HintFmt(
-                                % concatStringsSep<StringSet>(", ", m.systemTypes)
+                                "\n([%s], %s, [%s], [%s])",
-                                % m.maxJobs
+                                concatStringsSep<StringSet>(", ", m.systemTypes),
-                                % concatStringsSep<StringSet>(", ", m.supportedFeatures)
+                                m.maxJobs,
-                                % concatStringsSep<StringSet>(", ", m.mandatoryFeatures);
+                                concatStringsSep<StringSet>(", ", m.supportedFeatures),
+                                concatStringsSep<StringSet>(", ", 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<StringSet>(", ", requiredFeatures),
+                            machines.size(),
+                            Uncolored(machinesFormatted)
+                       );
 
                         printMsg(couldBuildLocally ? lvlChatty : lvlWarn, error.str());
 

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;

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);
 
 }

src/libutil/fmt.hh

@@ -5,43 +5,94 @@
 #include <string>
 #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:
+ * 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%`.
- * formatHelper(formatter, a_0, ..., a_n)
- * formatter % a_0 % ... % a_n
- * ```
- *
- * With a single argument, `formatHelper(s)` is a no-op.
  */
-template<class F>
+template<class T>
-inline void formatHelper(F & f)
+struct Magenta
-{ }
-
-template<class F, typename T, typename... Args>
-inline void formatHelper(F & f, const T & x, const Args & ... args)
 {
-    // Interpolate one argument and then recurse.
+    Magenta(const T & s) : value(s) {}
-    formatHelper(f % x, args...);
+    const T & value;
+};
+
+template<class T>
+std::ostream & operator<<(std::ostream & out, const Magenta<T> & 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<class T>
+struct Uncolored
+{
+    Uncolored(const T & s) : value(s) {}
+    const T & value;
+};
+
+template<class T>
+std::ostream & operator<<(std::ostream & out, const Uncolored<T> & 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::all_error_bits ^ boost::io::too_many_args_bit ^ boost::io::too_few_args_bit
-        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<typename... Args>
+    HintFmt(boost::format && fmt, const Args &... args) : fmt(std::move(fmt))
+    {
+        setExceptions(fmt);
+        (*this % ... % args);
+    }
+
+    template<class T>
+    HintFmt & operator%(const T & value)
+    {
+        fmt % Magenta(value);
+        return *this;
+    }
+
+    template<class T>
+    HintFmt & operator%(const Uncolored<T> & 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<typename... Args>
-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);
+    fmt_internal::setExceptions(f);
-    formatHelper(f, args...);
+    (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 <class T>
-struct Magenta
-{
-    Magenta(const T &s) : value(s) {}
-    const T & value;
-};
-
-template <class T>
-std::ostream & operator<<(std::ostream & out, const Magenta<T> & 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 <class T>
-struct Uncolored
-{
-    Uncolored(const T & s) : value(s) {}
-    const T & value;
-};
-
-template <class T>
-std::ostream & operator<<(std::ostream & out, const Uncolored<T> & 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(const std::string & literal) : HintFmt("%s", Uncolored(literal)) {}
-        : HintFmt("%s", Uncolored(literal))
-    { }
-
-    static HintFmt fromFormatString(const std::string & format) {
-        return HintFmt(boost::format(format));
-    }
 
     /**
      * Interpolate the given arguments into the format string.
      */
     template<typename... Args>
-    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)
+    HintFmt(const HintFmt & hf) : fmt(hf.fmt) {}
-        : fmt(hf.fmt)
-    { }
 
     template<typename... Args>
-    HintFmt(boost::format && fmt, const Args & ... args)
+    HintFmt(boost::format && fmt, const Args &... args)
-        : fmt(std::move(fmt))
+        : fmt(fmt_internal::HintFmt(std::move(fmt), args...).into_format())
     {
-        setExceptions(fmt);
+        if (this->fmt.remaining_args() != 0) {
-        formatHelper(*this, args...);
+            throw boost::io::too_few_args(
-    }
+                this->fmt.bound_args() + this->fmt.fed_args(), this->fmt.expected_args()
-
+            );
-    template<class T>
+        }
-    HintFmt & operator%(const T & value)
-    {
-        fmt % Magenta(value);
-        return *this;
-    }
-
-    template<class T>
-    HintFmt & operator%(const Uncolored<T> & value)
-    {
-        fmt % value.value;
-        return *this;
     }
 
     std::string str() const

src/libutil/logging.hh

@@ -242,9 +242,7 @@ extern Verbosity verbosity;
 template<typename... Args>
 inline void warn(const std::string & fs, const Args & ... args)
 {
-    boost::format f(fs);
+    logger->warn(HintFmt(fs, args...).str());
-    formatHelper(f, args...);
-    logger->warn(f.str());
 }
 
 #define warnOnce(haveWarned, args...) \

tests/unit/libutil/fmt.cc

@@ -0,0 +1,23 @@
+#include "fmt.hh"
+#include "ansicolor.hh"
+
+#include <gtest/gtest.h>
+
+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);
+}
+
+}