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

Conversation

polyvertex
Copy link
Contributor

@polyvertex polyvertex commented Jan 3, 2017

A buffer-writer utility pair that allows to append formatted string to an external "standard-compliant" container such as std::vector or std::basic_string.

Container type requirements:

  • typedef CharT value_type;
  • std::size_t size() const
  • std::size_t resize() const
  • CharT& operator[](std::size_t index)
  • contiguous buffer

Cons:

  • Adds an extra header file

Pros:

  • Directly writes into the external container, passed by ref
  • Implicit use of container's allocator
  • "Append" feature available for free
  • More generic than the buffer-writer pair implemented in fmt/string.h
  • No modification applied to fmt itself

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Looks great! Just two minor comments and please squash the commits (or I can do it during merge).


public:
explicit ContainerBuffer(Container& container)
: container_(container), initial_size_(container.size()) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to initialize size_ from container.size() and get rid of initial_size_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I'm not sure sure how grow() should work in case container_ is not empty (i.e. in append mode)? Even if I initialize size_ and capacity_ from the constructor, I fail to see how I can initialize ptr_ safely from there. Did I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

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

ptr_ points to the beginning of the buffer (container's data), size_ is the size of the filled part of the buffer and capacity_ is the size + free space. So if you initialize size_ append should Just Work, because the new output operations will write past the initial size, unless I'm missing something myself.

Copy link
Contributor Author

@polyvertex polyvertex Jan 3, 2017

Choose a reason for hiding this comment

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

This seems to be our Graal.

EDIT note that capacity_ is initialized to size_ instead of container_.capacity() to ensure grow() is triggered and the container is actually resized.

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

appender.write(format, args);
}
FMT_VARIADIC(std::vector<char>, vecformat,
std::vector<char>&, fmt::BasicCStringRef<char>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check if example compiles. It should probably use FMT_VARIADIC_VOID instead of FMT_VARIADIC.

@polyvertex
Copy link
Contributor Author

Commits squashed! 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.

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!

@vitaut vitaut merged commit e0251fd into fmtlib:master Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants