-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fmt::ostream - aggregate buffer instead of inheriting it #3139
Changes from 3 commits
ef40748
e75f9b8
4e0c866
07ca088
8bf75f9
7fa287d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -379,56 +379,76 @@ struct ostream_params { | |
# endif | ||
}; | ||
|
||
FMT_END_DETAIL_NAMESPACE | ||
|
||
// Added {} below to work around default constructor error known to | ||
// occur in Xcode versions 7.2.1 and 8.2.1. | ||
constexpr detail::buffer_size buffer_size{}; | ||
|
||
/** A fast output stream which is not thread-safe. */ | ||
class FMT_API ostream final : private detail::buffer<char> { | ||
private: | ||
template <typename Char> class file_buffer final : public detail::buffer<Char> { | ||
file file_; | ||
|
||
void grow(size_t) override; | ||
|
||
ostream(cstring_view path, const detail::ostream_params& params) | ||
: file_(path, params.oflag) { | ||
set(new char[params.buffer_size], params.buffer_size); | ||
void grow(size_t) override { | ||
if (this->size() == this->capacity()) flush(); | ||
} | ||
|
||
public: | ||
ostream(ostream&& other) | ||
: detail::buffer<char>(other.data(), other.size(), other.capacity()), | ||
file_buffer(cstring_view path, const detail::ostream_params& params) | ||
: file_(path, params.oflag) { | ||
detail::buffer<Char>::set(new Char[params.buffer_size], params.buffer_size); | ||
} | ||
file_buffer(file_buffer&& other) | ||
: detail::buffer<Char>(other.data(), other.size(), other.capacity()), | ||
file_(std::move(other.file_)) { | ||
other.clear(); | ||
other.set(nullptr, 0); | ||
} | ||
~ostream() { | ||
~file_buffer() { | ||
flush(); | ||
delete[] data(); | ||
delete[] detail::buffer<Char>::data(); | ||
} | ||
|
||
void flush() { | ||
if (size() == 0) return; | ||
file_.write(data(), size()); | ||
clear(); | ||
if (detail::buffer<Char>::size() == 0) return; | ||
file_.write( | ||
detail::buffer<Char>::data(), | ||
detail::buffer<Char>::size() * sizeof(detail::buffer<Char>::data()[0])); | ||
detail::buffer<Char>::clear(); | ||
} | ||
|
||
template <typename... T> | ||
friend ostream output_file(cstring_view path, T... params); | ||
|
||
void close() { | ||
flush(); | ||
file_.close(); | ||
} | ||
}; | ||
|
||
FMT_END_DETAIL_NAMESPACE | ||
|
||
// Added {} below to work around default constructor error known to | ||
// occur in Xcode versions 7.2.1 and 8.2.1. | ||
constexpr detail::buffer_size buffer_size{}; | ||
|
||
/** A fast output stream which is not thread-safe. */ | ||
class FMT_API ostream final { | ||
Youw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private: | ||
FMT_MSC_WARNING(suppress : 4251) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I.e. MSVC doesn't like that |
||
detail::file_buffer<char> buffer_; | ||
|
||
ostream(cstring_view path, const detail::ostream_params& params) | ||
: buffer_(path, params) {} | ||
|
||
public: | ||
ostream(ostream&& other) : buffer_(std::move(other.buffer_)) {} | ||
|
||
~ostream(); | ||
|
||
void flush() { buffer_.flush(); } | ||
|
||
template <typename... T> | ||
friend ostream output_file(cstring_view path, T... params); | ||
|
||
void close() { buffer_.close(); } | ||
|
||
/** | ||
Formats ``args`` according to specifications in ``fmt`` and writes the | ||
output to the file. | ||
*/ | ||
template <typename... T> void print(format_string<T...> fmt, T&&... args) { | ||
vformat_to(detail::buffer_appender<char>(*this), fmt, | ||
vformat_to(detail::buffer_appender<char>(buffer_), fmt, | ||
fmt::make_format_args(args...)); | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -366,8 +366,6 @@ long getpagesize() { | |
# endif | ||
} | ||
|
||
FMT_API void ostream::grow(size_t) { | ||
if (this->size() == this->capacity()) flush(); | ||
} | ||
ostream::~ostream() = default; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it need to be outlined? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it guess that's my old habit - always anchor class by its d-tor maybe that is not the case for this particular class - but that's the case for polymorphic classes |
||
#endif // FMT_USE_FCNTL | ||
FMT_END_NAMESPACE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be a template because file I/O is char-based.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is exactly what I've tried in the previous commit.
But compilers generally are not happy about it.
I simply took the same approach as with
memory_buffer
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to fix that warning by outlining some virtual method.
memory_buffer
is different because it has to be a template.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That I did and got a different problem - the v-table gets anchored into a shared library but not exported, which makes it unavailable outside of a shared library.
Initially I thought that the only fix would be to export the entire class (to export a v-table) which is the opposite of this entire change. But after giving it a second though - I think it would be enough to outline c-tor and other special functions. Let me try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done - it worked