BIND 10 trac901, updated. 4ff5a9f61d9ad1c96468556771562f1dbad7ec98 [trac901] Minor modifications based on review
BIND 10 source code commits
bind10-changes at lists.isc.org
Sun May 8 20:14:14 UTC 2011
The branch, trac901 has been updated
via 4ff5a9f61d9ad1c96468556771562f1dbad7ec98 (commit)
from 9a8870c515be763e8be63cdc28231883601867d4 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 4ff5a9f61d9ad1c96468556771562f1dbad7ec98
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Sun May 8 22:13:45 2011 +0200
[trac901] Minor modifications based on review
-----------------------------------------------------------------------
Summary of changes:
src/lib/log/log_formatter.h | 46 +++++++++++----------------
src/lib/log/macros.h | 2 +
src/lib/log/tests/log_formatter_unittest.cc | 24 +++++---------
3 files changed, 29 insertions(+), 43 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/log/log_formatter.h b/src/lib/log/log_formatter.h
index d33a9be..bc4340c 100644
--- a/src/lib/log/log_formatter.h
+++ b/src/lib/log/log_formatter.h
@@ -45,16 +45,12 @@ replacePlaceholder(std::string* message, const std::string& replacement,
/// no .arg call on the object, it is destroyed right away and we use the
/// destructor to output the text (but only in case we should output anything).
///
-/// If there's an .arg call, it replaces a placeholder with the argument
-/// converted to string and produces another Formatter. We mark the current
-/// Formatter so it won't output anything in it's destructor and the task
-/// to do the output is moved onto the new object. Again, the last one in
-/// the chain is destroyed without any modification and does the real output.
+/// If there's an .arg call, we return reference to the same object, so another
+/// .arg can be called on it. After the last .arg call is done, the object is
+/// destroyed and, again, we can produce the output.
///
/// Of course, if the logging is turned off, we don't bother with any replacing
-/// and just return new empty Formatter. As everything here is in the header
-/// file, compiler should be able to easily optimise most of the work with
-/// creating and destroying objects and simply do the replacing only.
+/// and just return.
///
/// User of logging code should not really care much about this class, only
/// call the .arg method to generate the correct output.
@@ -62,6 +58,15 @@ replacePlaceholder(std::string* message, const std::string& replacement,
/// The class is a template to allow easy testing. Also, we want everything
/// here in the header anyway and it doesn't depend on the details of what
/// Logger really is, so it doesn't hurt anything.
+///
+/// Also, if you are interested in the internals, you might find the copy
+/// constructor a bit strange. It deactivates the original formatter. We don't
+/// really want to support copying of the Formatter by user, but C++ needs a
+/// copy constructor when returning from the logging functions, so we need one.
+/// And if we did not deactivate the original Formatter, that one would get
+/// destroyed before any call to .arg, producing an output, and then the one
+/// the .arg calls are called on would get destroyed as well, producing output
+/// again. So, think of this behaviour as soul moving from one to another.
template<class Logger> class Formatter {
private:
/// \brief The logger we will use to output the final message
@@ -71,7 +76,7 @@ private:
/// \brief The messages with %1, %2... placeholders
std::string* message_;
/// \brief Which will be the next placeholder to replace
- const unsigned nextPlaceholder_;
+ unsigned nextPlaceholder_;
/// \brief Should we do output?
mutable bool active_;
Formatter& operator =(const Formatter& other);
@@ -122,8 +127,8 @@ public:
~ Formatter() {
if (active_) {
logger_->output(prefix_, *message_);
+ delete message_;
}
- delete message_;
}
/// \brief Replaces another placeholder
///
@@ -132,32 +137,19 @@ public:
/// only produces another inactive formatter.
///
/// \param arg The argument to place into the placeholder.
- template<class Arg> Formatter arg(const Arg& value) {
+ 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) {
+ 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;
- 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>());
+ replacePlaceholder(message_, arg, nextPlaceholder_ ++);
}
+ return (*this);
}
};
diff --git a/src/lib/log/macros.h b/src/lib/log/macros.h
index 4a5ac19..3128131 100644
--- a/src/lib/log/macros.h
+++ b/src/lib/log/macros.h
@@ -15,6 +15,8 @@
#ifndef __LOG_MACROS_H
#define __LOG_MACROS_H
+#include <log/logger.h>
+
/// \brief Macro to conveniently test debug output and log it
#define LOG_DEBUG(LOGGER, LEVEL, MESSAGE) \
if (!(LOGGER).isDebugEnabled((LEVEL))) { \
diff --git a/src/lib/log/tests/log_formatter_unittest.cc b/src/lib/log/tests/log_formatter_unittest.cc
index 0b7e7e5..351bf24 100644
--- a/src/lib/log/tests/log_formatter_unittest.cc
+++ b/src/lib/log/tests/log_formatter_unittest.cc
@@ -47,8 +47,7 @@ TEST_F(FormatterTest, inactive) {
// substitution yet
TEST_F(FormatterTest, active) {
Formatter("TEST", s("Text of message"), 1, *this);
- ASSERT_LE(1, outputs.size());
- EXPECT_EQ(1, outputs.size());
+ ASSERT_EQ(1, outputs.size());
EXPECT_STREQ("TEST", outputs[0].first);
EXPECT_EQ("Text of message", outputs[0].second);
}
@@ -64,16 +63,14 @@ TEST_F(FormatterTest, stringArg) {
{
SCOPED_TRACE("C++ string");
Formatter("TEST", s("Hello %1"), 1, *this).arg(string("World"));
- ASSERT_LE(1, outputs.size());
- EXPECT_EQ(1, outputs.size());
+ ASSERT_EQ(1, outputs.size());
EXPECT_STREQ("TEST", outputs[0].first);
EXPECT_EQ("Hello World", outputs[0].second);
}
{
SCOPED_TRACE("C++ string");
Formatter("TEST", s("Hello %1"), 1, *this).arg(string("Internet"));
- ASSERT_LE(2, outputs.size());
- EXPECT_EQ(2, outputs.size());
+ ASSERT_EQ(2, outputs.size());
EXPECT_STREQ("TEST", outputs[1].first);
EXPECT_EQ("Hello Internet", outputs[1].second);
}
@@ -82,8 +79,7 @@ TEST_F(FormatterTest, stringArg) {
// Can convert to string
TEST_F(FormatterTest, intArg) {
Formatter("TEST", s("The answer is %1"), 1, *this).arg(42);
- ASSERT_LE(1, outputs.size());
- EXPECT_EQ(1, outputs.size());
+ ASSERT_EQ(1, outputs.size());
EXPECT_STREQ("TEST", outputs[0].first);
EXPECT_EQ("The answer is 42", outputs[0].second);
}
@@ -92,8 +88,7 @@ TEST_F(FormatterTest, intArg) {
TEST_F(FormatterTest, multiArg) {
Formatter("TEST", s("The %2 are %1"), 1, *this).arg("switched").
arg("arguments");
- ASSERT_LE(1, outputs.size());
- EXPECT_EQ(1, outputs.size());
+ ASSERT_EQ(1, outputs.size());
EXPECT_STREQ("TEST", outputs[0].first);
EXPECT_EQ("The arguments are switched", outputs[0].second);
}
@@ -102,8 +97,7 @@ TEST_F(FormatterTest, multiArg) {
TEST_F(FormatterTest, missingPlace) {
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());
+ ASSERT_EQ(1, outputs.size());
EXPECT_STREQ("TEST", outputs[0].first);
EXPECT_EQ("Missing the first argument "
"@@Missing placeholder %1 for 'missing'@@", outputs[0].second);
@@ -113,8 +107,7 @@ TEST_F(FormatterTest, missingPlace) {
TEST_F(FormatterTest, multiPlaceholder) {
Formatter("TEST", s("The %1 is the %1"), 1, *this).
arg("first rule of tautology club");
- ASSERT_LE(1, outputs.size());
- EXPECT_EQ(1, outputs.size());
+ ASSERT_EQ(1, outputs.size());
EXPECT_STREQ("TEST", outputs[0].first);
EXPECT_EQ("The first rule of tautology club is "
"the first rule of tautology club", outputs[0].second);
@@ -124,8 +117,7 @@ TEST_F(FormatterTest, multiPlaceholder) {
TEST_F(FormatterTest, noRecurse) {
// If we recurse, this will probably eat all the memory and crash
Formatter("TEST", s("%1"), 1, *this).arg("%1 %1");
- ASSERT_LE(1, outputs.size());
- EXPECT_EQ(1, outputs.size());
+ ASSERT_EQ(1, outputs.size());
EXPECT_STREQ("TEST", outputs[0].first);
EXPECT_EQ("%1 %1", outputs[0].second);
}
More information about the bind10-changes
mailing list