Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BasicContainerWriter utility added #450

Merged
merged 1 commit into from
Jan 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ store output elsewhere by subclassing `~fmt::BasicWriter`.
.. doxygenclass:: fmt::BasicStringWriter
:members:

.. doxygenclass:: fmt::BasicContainerWriter
:members:

.. doxygenfunction:: bin(int)

.. doxygenfunction:: oct(int)
Expand Down
3 changes: 2 additions & 1 deletion doc/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ def build_docs(version='dev', **kwargs):
GENERATE_MAN = NO
GENERATE_RTF = NO
CASE_SENSE_NAMES = NO
INPUT = {0}/format.h {0}/ostream.h {0}/printf.h {0}/string.h
INPUT = {0}/container.h {0}/format.h {0}/ostream.h \
{0}/printf.h {0}/string.h
QUIET = YES
JAVADOC_AUTOBRIEF = YES
AUTOLINK_SUPPORT = NO
Expand Down
4 changes: 2 additions & 2 deletions fmt/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Define the fmt library, its includes and the needed defines.
# *.cc are added to FMT_HEADERS for the header-only configuration.
set(FMT_HEADERS format.h format.cc ostream.h ostream.cc printf.h printf.cc
string.h time.h)
set(FMT_HEADERS container.h format.h format.cc ostream.h ostream.cc printf.h
printf.cc string.h time.h)
if (HAVE_OPEN)
set(FMT_HEADERS ${FMT_HEADERS} posix.h)
set(FMT_SOURCES ${FMT_SOURCES} posix.cc)
Expand Down
82 changes: 82 additions & 0 deletions fmt/container.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
Formatting library for C++ - standard container utilities

Copyright (c) 2012 - 2016, Victor Zverovich
All rights reserved.

For the license information refer to format.h.
*/

#ifndef FMT_CONTAINER_H_
#define FMT_CONTAINER_H_

#include "format.h"

namespace fmt {

namespace internal {

/**
\rst
A "buffer" that appends data to a standard container (e.g. typically a
``std::vector`` or ``std::basic_string``).
\endrst
*/
template <typename Container>
class ContainerBuffer : public Buffer<typename Container::value_type> {
private:
Container& container_;

protected:
virtual void grow(std::size_t size) FMT_OVERRIDE {
container_.resize(size);
this->ptr_ = &container_[0];
this->capacity_ = size;
}

public:
explicit ContainerBuffer(Container& container) : container_(container) {
this->size_ = container_.size();
if (this->size_ > 0) {
this->ptr_ = &container_[0];
this->capacity_ = this->size_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like capacity will be uninitialized if size is zero. Could you add a test case that catches this? Also I'd omit the zero check altogether.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the corrected version of the example still uses FMT_VARIADIC instead of FMT_VARIADIC_VOID because the later does not accept extra parameters.

Does it compile? =)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... I'd omit the zero check altogether.

The point of the test is to avoid assuming that it is safe to initialize ptr_ (thus assuming &container_[0] is always safe) before being sure it is (i.e. after the container_.resize() call). Although admittedly it should be fine with standard containers with most compilers, but we cannot assume Container type is standard here, or derived from a standard.

Looks like capacity will be uninitialized if size is zero.

Doesn't fmt::Buffer's default constructor set capacity_ to zero?

Could you add a test case that catches this?

You mean like TEST(BasicContainerWriterTest, String), which already tests that case unless I misunderstood you, but explicitly for ContainerBuffer?

Does it compile?

The corrected example compiles on MSVC2015 SP3. Tested before committing of course :)
I cannot confirm it compiles just fine on other configs though, or that it is even C++98 compliant, like it's supposed to be for fmt. Do note however I followed the same example pattern found in the documentation of the FMT_VARIADIC macro, which has a void return type as well.

Also, please note MSVC2015 SP3 is the only compiler I have access to ATM since I have quite limited access and bandwidth to Internet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good points. Thanks!

}
}
};
} // namespace internal

/**
\rst
This class template provides operations for formatting and appending data
to a standard *container* like ``std::vector`` or ``std::basic_string``.

**Example**::

void vecformat(std::vector<char>& dest, fmt::BasicCStringRef<char> format,
fmt::ArgList args) {
fmt::BasicContainerWriter<std::vector<char> > appender(dest);
appender.write(format, args);
}
FMT_VARIADIC(void, vecformat, std::vector<char>&,
fmt::BasicCStringRef<char>);
\endrst
*/
template <class Container>
class BasicContainerWriter
: public BasicWriter<typename Container::value_type> {
private:
internal::ContainerBuffer<Container> buffer_;

public:
/**
\rst
Constructs a :class:`fmt::BasicContainerWriter` object.
\endrst
*/
explicit BasicContainerWriter(Container& dest)
: BasicWriter<typename Container::value_type>(buffer_), buffer_(dest) {}
};

} // namespace fmt

#endif // FMT_CONTAINER_H_
3 changes: 2 additions & 1 deletion test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ function(add_fmt_test name)
endfunction()

add_fmt_test(assert-test)
add_fmt_test(container-test)
add_fmt_test(gtest-extra-test)
add_fmt_test(format-test)
add_fmt_test(format-impl-test)
Expand Down Expand Up @@ -139,7 +140,7 @@ if (FMT_PEDANTIC)
"${CMAKE_CURRENT_BINARY_DIR}/compile-test"
--build-generator ${CMAKE_GENERATOR}
--build-makeprogram ${CMAKE_MAKE_PROGRAM}
--build-options
--build-options
"-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}"
"-DCPP11_FLAG=${CPP11_FLAG}"
"-DSUPPORTS_USER_DEFINED_LITERALS=${SUPPORTS_USER_DEFINED_LITERALS}")
Expand Down
94 changes: 94 additions & 0 deletions test/container-test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
Tests of container utilities

Copyright (c) 2012 - 2016, Victor Zverovich
All rights reserved.

For the license information refer to format.h.
*/

#include "fmt/container.h"
#include "gtest/gtest.h"

using fmt::internal::ContainerBuffer;

TEST(ContainerBufferTest, Empty) {
std::string data;
ContainerBuffer<std::string> buffer(data);
EXPECT_EQ(0u, buffer.size());
EXPECT_EQ(0u, buffer.capacity());
}

TEST(ContainerBufferTest, Reserve) {
std::string data;
ContainerBuffer<std::string> buffer(data);
std::size_t capacity = std::string().capacity() + 10;
buffer.reserve(capacity);
EXPECT_EQ(0u, buffer.size());
EXPECT_EQ(capacity, buffer.capacity());
}

TEST(ContainerBufferTest, Resize) {
std::string data;
ContainerBuffer<std::string> buffer(data);
std::size_t size = std::string().capacity() + 10;
buffer.resize(size);
EXPECT_EQ(size, buffer.size());
EXPECT_EQ(size, buffer.capacity());
}

TEST(ContainerBufferTest, Append) {
std::string data("Why so");
const std::string serious(" serious");
ContainerBuffer<std::string> buffer(data);
buffer.append(serious.c_str(), serious.c_str() + serious.length());
EXPECT_EQ("Why so serious", data);
EXPECT_EQ(data.length(), buffer.size());
}

TEST(BasicContainerWriterTest, String) {
std::string data;
fmt::BasicContainerWriter<std::string> out(data);
out << "The answer is " << 42 << "\n";
EXPECT_EQ("The answer is 42\n", data);
EXPECT_EQ(17u, out.size());
}

TEST(BasicContainerWriterTest, WString) {
std::wstring data;
fmt::BasicContainerWriter<std::wstring> out(data);
out << "The answer is " << 42 << "\n";
EXPECT_EQ(L"The answer is 42\n", data);
EXPECT_EQ(17u, out.size());
}

TEST(BasicContainerWriterTest, Vector) {
std::vector<char> data;
fmt::BasicContainerWriter<std::vector<char> > out(data);
out << "The answer is " << 42 << "\n";
EXPECT_EQ(17u, data.size());
EXPECT_EQ(out.size(), data.size());
}

TEST(BasicContainerWriterTest, StringAppend) {
std::string data("The");
fmt::BasicContainerWriter<std::string> out(data);
EXPECT_EQ(3u, data.size());
EXPECT_EQ(3u, out.size());
out << " answer is " << 42 << "\n";
EXPECT_EQ("The answer is 42\n", data);
EXPECT_EQ(17u, out.size());
}

TEST(BasicContainerWriterTest, VectorAppend) {
std::vector<char> data;
data.push_back('T');
data.push_back('h');
data.push_back('e');
fmt::BasicContainerWriter<std::vector<char> > out(data);
EXPECT_EQ(3u, data.size());
EXPECT_EQ(3u, out.size());
out << " answer is " << 42 << "\n";
EXPECT_EQ(17u, data.size());
EXPECT_EQ(17u, out.size());
}