[trac901] Replacing in the formatter

src/lib/log/Makefile.am

@@ -21,7 +21,7 @@ liblog_la_SOURCES += message_initializer.cc message_initializer.h
 liblog_la_SOURCES += message_reader.cc message_reader.h
 liblog_la_SOURCES += message_types.h
 liblog_la_SOURCES += root_logger_name.cc root_logger_name.h
-liblog_la_SOURCES += log_formatter.h
+liblog_la_SOURCES += log_formatter.h log_formatter.cc
 
 EXTRA_DIST  = README
 EXTRA_DIST += messagedef.mes

src/lib/log/log_formatter.h

@@ -16,10 +16,19 @@
 #define __LOG_FORMMATER_H
 
 #include <string>
+#include <boost/lexical_cast.hpp>
 
 namespace isc {
 namespace log {
 
+/// \brief The internal replacement routine
+///
+/// This is used internally by the Formatter. Replaces a placeholder
+/// in the message by replacement. If the placeholder is not found,
+/// it adds a complain at the end.
+void replacePlaceholder(std::string* message, const std::string& replacement,
+                        const unsigned placeholder);
+
 ///
 /// \brief The log message formatter
 ///
@@ -59,7 +68,7 @@ private:
     /// \brief Prefix (eg. "ERROR", "DEBUG" or like that)
     const char* prefix_;
     /// \brief The messages with %1, %2... placeholders
-    const std::string message_;
+    std::string* message_;
     /// \brief Which will be the next placeholder to replace
     const unsigned nextPlaceholder_;
     /// \brief Should we do output?
@@ -72,13 +81,17 @@ public:
     /// This will create a formatter in active mode -- the one when it
     /// will generate output.
     ///
+    /// It is not expected to be called by user of logging system directly.
+    ///
     /// \param prefix The prefix, like "ERROR" or "DEBUG"
-    /// \param message The message with placeholders
+    /// \param message The message with placeholders. We take ownership of
+    ///     it and we will modify the string. Must not be NULL and it's
+    ///     not checked.
     /// \param nextPlaceholder It is the number of next placeholder which
     ///     should be replaced. It should be called with 1, higher numbers
     ///     are used internally in the chain.
     /// \param logger The logger where the final output will go.
-    Formatter(const char* prefix, const std::string& message,
+    Formatter(const char* prefix, std::string* message,
               const unsigned nextPlaceholder, Logger& logger) :
         logger_(&logger), prefix_(prefix), message_(message),
         nextPlaceholder_(nextPlaceholder), active_(true)
@@ -86,8 +99,11 @@ public:
     }
     /// \brief Constructor of "inactive" formatter
     ///
+    /// It is not expected to be called by user of the logging system directly.
+    ///
     /// This will create a formatter that produces no output.
     Formatter() :
+        message_(NULL),
         nextPlaceholder_(0),
         active_(false)
     {
@@ -97,8 +113,9 @@ public:
     /// This is the place where output happens if the formatter is active.
     ~ Formatter() {
         if (active_) {
-            logger_->output(prefix_, message_);
+            logger_->output(prefix_, *message_);
         }
+        delete message_;
     }
     /// \brief Replaces another placeholder
     ///
@@ -107,11 +124,29 @@ public:
     /// only produces another inactive formatter.
     ///
     /// \param arg The argument to place into the placeholder.
-    template<class Arg> Formatter arg(const Arg&) {
+    template<class Arg> Formatter arg(const Arg& value) {
+        return (arg(boost::lexical_cast<std::string>(value)));
+    }
+    /// \brief String version of arg.
+    Formatter arg(const std::string& arg) {
         if (active_) {
+            // FIXME: This logic has a problem. If we had a message like
+            // "%1 %2" and called .arg("%2").arg(42), we would get "42 %2".
+            // But we consider this to be rare enough not to complicate
+            // matters.
             active_ = false;
-            return (Formatter<Logger>(prefix_, message_, nextPlaceholder_ + 1,
-                                      *logger_));
+            replacePlaceholder(message_, arg, nextPlaceholder_);
+            std::string *message(message_);
+            message_ = NULL;
+            try {
+                return (Formatter<Logger>(prefix_, message,
+                                          nextPlaceholder_ + 1, *logger_));
+            }
+            // Make sure the memory is not leaked on stuff like bad_alloc
+            catch (...) {
+                delete message;
+                throw;
+            }
         } else {
             return (Formatter<Logger>());
         }

src/lib/log/tests/log_formatter_unittest.cc

@@ -31,6 +31,10 @@ public:
     void output(const char* prefix, const string& message) {
         outputs.push_back(Output(prefix, message));
     }
+    // Just shortcut for new string
+    string* s(const char* text) {
+        return (new string(text));
+    }
 };
 
 // Create an inactive formatter and check it doesn't produce any output
@@ -42,7 +46,7 @@ TEST_F(FormatterTest, inactive) {
 // Create an active formatter and check it produces output. Does no arg
 // substitution yet
 TEST_F(FormatterTest, active) {
-    Formatter("TEST", "Text of message", 1, *this);
+    Formatter("TEST", s("Text of message"), 1, *this);
     ASSERT_LE(1, outputs.size());
     EXPECT_EQ(1, outputs.size());
     EXPECT_STREQ("TEST", outputs[0].first);
@@ -59,7 +63,7 @@ TEST_F(FormatterTest, inactiveArg) {
 TEST_F(FormatterTest, stringArg) {
     {
         SCOPED_TRACE("C++ string");
-        Formatter("TEST", "Hello %1", 1, *this).arg(string("World"));
+        Formatter("TEST", s("Hello %1"), 1, *this).arg(string("World"));
         ASSERT_LE(1, outputs.size());
         EXPECT_EQ(1, outputs.size());
         EXPECT_STREQ("TEST", outputs[0].first);
@@ -67,7 +71,7 @@ TEST_F(FormatterTest, stringArg) {
     }
     {
         SCOPED_TRACE("C++ string");
-        Formatter("TEST", "Hello %1", 1, *this).arg(string("Internet"));
+        Formatter("TEST", s("Hello %1"), 1, *this).arg(string("Internet"));
         ASSERT_LE(2, outputs.size());
         EXPECT_EQ(2, outputs.size());
         EXPECT_STREQ("TEST", outputs[1].first);
@@ -77,7 +81,7 @@ TEST_F(FormatterTest, stringArg) {
 
 // Can convert to string
 TEST_F(FormatterTest, intArg) {
-    Formatter("TEST", "The answer is %1", 1, *this).arg(42);
+    Formatter("TEST", s("The answer is %1"), 1, *this).arg(42);
     ASSERT_LE(1, outputs.size());
     EXPECT_EQ(1, outputs.size());
     EXPECT_STREQ("TEST", outputs[0].first);
@@ -86,7 +90,7 @@ TEST_F(FormatterTest, intArg) {
 
 // Can use multiple arguments at different places
 TEST_F(FormatterTest, multiArg) {
-    Formatter("TEST", "The %2 are %1", 1, *this).arg("switched").
+    Formatter("TEST", s("The %2 are %1"), 1, *this).arg("switched").
         arg("arguments");
     ASSERT_LE(1, outputs.size());
     EXPECT_EQ(1, outputs.size());
@@ -96,8 +100,8 @@ TEST_F(FormatterTest, multiArg) {
 
 // Can survive and complains if placeholder is missing
 TEST_F(FormatterTest, missingPlace) {
-    Formatter("TEST", "Missing the first %2", 1, *this).arg("missing").
-        arg("argument");
+    EXPECT_NO_THROW(Formatter("TEST", s("Missing the first %2"), 1, *this).
+                    arg("missing").arg("argument"));
     ASSERT_LE(1, outputs.size());
     EXPECT_EQ(1, outputs.size());
     EXPECT_STREQ("TEST", outputs[0].first);