From ef4074802a2b528d3293f4e1e3ac6da0deeece20 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Thu, 13 Oct 2022 22:42:27 +0300 Subject: [PATCH 1/6] fmt::ostream - aggregate buffer instead of inheriting it Some MSVC-specific behavior: When class fmt::ostream inherits detail::buffer - the last gets implicitly exported when fmt is built as a shared library. Unless os.h is included, the compiler assumes detail::buffer is not externally exported and instantiets a local copy of it, which causes ODR violation. With aggregation - there is no extra exporting of detail::buffer symbols. --- include/fmt/os.h | 54 +++++++++++++++++++++++++++++++----------------- src/os.cc | 4 +++- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/include/fmt/os.h b/include/fmt/os.h index 8e697ec4e6fa..386649509e76 100644 --- a/include/fmt/os.h +++ b/include/fmt/os.h @@ -379,32 +379,23 @@ 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 { - private: +class ostream_buffer final : public detail::buffer { file file_; - void grow(size_t) override; + FMT_API void grow(size_t) override; - ostream(cstring_view path, const detail::ostream_params& params) + public: + ostream_buffer(cstring_view path, const detail::ostream_params& params) : file_(path, params.oflag) { set(new char[params.buffer_size], params.buffer_size); } - - public: - ostream(ostream&& other) + ostream_buffer(ostream_buffer&& other) : detail::buffer(other.data(), other.size(), other.capacity()), file_(std::move(other.file_)) { other.clear(); other.set(nullptr, 0); } - ~ostream() { + ~ostream_buffer() { flush(); delete[] data(); } @@ -415,20 +406,45 @@ class FMT_API ostream final : private detail::buffer { clear(); } - template - 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 { + private: + FMT_MSC_WARNING(suppress : 4251) + detail::ostream_buffer 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 + 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 void print(format_string fmt, T&&... args) { - vformat_to(detail::buffer_appender(*this), fmt, + vformat_to(detail::buffer_appender(buffer_), fmt, fmt::make_format_args(args...)); } }; diff --git a/src/os.cc b/src/os.cc index 08bc749ee302..c40fdaeac9d2 100644 --- a/src/os.cc +++ b/src/os.cc @@ -366,8 +366,10 @@ long getpagesize() { # endif } -FMT_API void ostream::grow(size_t) { +FMT_API void detail::ostream_buffer::grow(size_t) { if (this->size() == this->capacity()) flush(); } + +ostream::~ostream() = default; #endif // FMT_USE_FCNTL FMT_END_NAMESPACE From e75f9b8e38c9ca63f94e4cf9b4524ee89c14cb83 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Fri, 14 Oct 2022 11:42:46 +0300 Subject: [PATCH 2/6] Remove v-table anchor for detail::ostream_buffer --- include/fmt/os.h | 4 +++- src/os.cc | 4 ---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/include/fmt/os.h b/include/fmt/os.h index 386649509e76..2a7ebead314e 100644 --- a/include/fmt/os.h +++ b/include/fmt/os.h @@ -382,7 +382,9 @@ struct ostream_params { class ostream_buffer final : public detail::buffer { file file_; - FMT_API void grow(size_t) override; + void grow(size_t) override { + if (this->size() == this->capacity()) flush(); + } public: ostream_buffer(cstring_view path, const detail::ostream_params& params) diff --git a/src/os.cc b/src/os.cc index c40fdaeac9d2..06f400df7049 100644 --- a/src/os.cc +++ b/src/os.cc @@ -366,10 +366,6 @@ long getpagesize() { # endif } -FMT_API void detail::ostream_buffer::grow(size_t) { - if (this->size() == this->capacity()) flush(); -} - ostream::~ostream() = default; #endif // FMT_USE_FCNTL FMT_END_NAMESPACE From 4e0c8667d3bffc5e05abfcfecc5c2e6629336a39 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Fri, 14 Oct 2022 12:30:48 +0300 Subject: [PATCH 3/6] attempt to make the compiler happy --- include/fmt/os.h | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/include/fmt/os.h b/include/fmt/os.h index 2a7ebead314e..f9cee26273b9 100644 --- a/include/fmt/os.h +++ b/include/fmt/os.h @@ -379,7 +379,7 @@ struct ostream_params { # endif }; -class ostream_buffer final : public detail::buffer { +template class file_buffer final : public detail::buffer { file file_; void grow(size_t) override { @@ -387,25 +387,27 @@ class ostream_buffer final : public detail::buffer { } public: - ostream_buffer(cstring_view path, const detail::ostream_params& params) + file_buffer(cstring_view path, const detail::ostream_params& params) : file_(path, params.oflag) { - set(new char[params.buffer_size], params.buffer_size); + detail::buffer::set(new Char[params.buffer_size], params.buffer_size); } - ostream_buffer(ostream_buffer&& other) - : detail::buffer(other.data(), other.size(), other.capacity()), + file_buffer(file_buffer&& other) + : detail::buffer(other.data(), other.size(), other.capacity()), file_(std::move(other.file_)) { other.clear(); other.set(nullptr, 0); } - ~ostream_buffer() { + ~file_buffer() { flush(); - delete[] data(); + delete[] detail::buffer::data(); } void flush() { - if (size() == 0) return; - file_.write(data(), size()); - clear(); + if (detail::buffer::size() == 0) return; + file_.write( + detail::buffer::data(), + detail::buffer::size() * sizeof(detail::buffer::data()[0])); + detail::buffer::clear(); } void close() { @@ -424,7 +426,7 @@ constexpr detail::buffer_size buffer_size{}; class FMT_API ostream final { private: FMT_MSC_WARNING(suppress : 4251) - detail::ostream_buffer buffer_; + detail::file_buffer buffer_; ostream(cstring_view path, const detail::ostream_params& params) : buffer_(path, params) {} From 07ca0880354690d2cf08dbcbf935e529428b34d7 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Thu, 20 Oct 2022 19:57:45 +0300 Subject: [PATCH 4/6] non-template solution attempt --- include/fmt/os.h | 33 +++++++++------------------------ src/os.cc | 26 ++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/include/fmt/os.h b/include/fmt/os.h index f9cee26273b9..01a31bafe105 100644 --- a/include/fmt/os.h +++ b/include/fmt/os.h @@ -379,35 +379,20 @@ struct ostream_params { # endif }; -template class file_buffer final : public detail::buffer { +class file_buffer final : public detail::buffer { file file_; - void grow(size_t) override { - if (this->size() == this->capacity()) flush(); - } + FMT_API void grow(size_t) override; public: - file_buffer(cstring_view path, const detail::ostream_params& params) - : file_(path, params.oflag) { - detail::buffer::set(new Char[params.buffer_size], params.buffer_size); - } - file_buffer(file_buffer&& other) - : detail::buffer(other.data(), other.size(), other.capacity()), - file_(std::move(other.file_)) { - other.clear(); - other.set(nullptr, 0); - } - ~file_buffer() { - flush(); - delete[] detail::buffer::data(); - } + FMT_API file_buffer(cstring_view path, const detail::ostream_params& params); + FMT_API file_buffer(file_buffer&& other); + FMT_API ~file_buffer(); void flush() { - if (detail::buffer::size() == 0) return; - file_.write( - detail::buffer::data(), - detail::buffer::size() * sizeof(detail::buffer::data()[0])); - detail::buffer::clear(); + if (size() == 0) return; + file_.write(data(), size() * sizeof(data()[0])); + clear(); } void close() { @@ -426,7 +411,7 @@ constexpr detail::buffer_size buffer_size{}; class FMT_API ostream final { private: FMT_MSC_WARNING(suppress : 4251) - detail::file_buffer buffer_; + detail::file_buffer buffer_; ostream(cstring_view path, const detail::ostream_params& params) : buffer_(path, params) {} diff --git a/src/os.cc b/src/os.cc index 06f400df7049..999d2a755895 100644 --- a/src/os.cc +++ b/src/os.cc @@ -366,6 +366,32 @@ long getpagesize() { # endif } +FMT_BEGIN_DETAIL_NAMESPACE + +void file_buffer::grow(size_t) { + if (this->size() == this->capacity()) flush(); +} + +file_buffer::file_buffer(cstring_view path, + const detail::ostream_params& params) + : file_(path, params.oflag) { + set(new char[params.buffer_size], params.buffer_size); +} + +file_buffer::file_buffer(file_buffer&& other) + : detail::buffer(other.data(), other.size(), other.capacity()), + file_(std::move(other.file_)) { + other.clear(); + other.set(nullptr, 0); +} + +file_buffer::~file_buffer() { + flush(); + delete[] data(); +} + +FMT_END_DETAIL_NAMESPACE + ostream::~ostream() = default; #endif // FMT_USE_FCNTL FMT_END_NAMESPACE From 8bf75f97914c69c6e7b0e79160432250a7745fa3 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Sun, 23 Oct 2022 00:36:47 +0300 Subject: [PATCH 5/6] Apply suggestions from code review --- include/fmt/os.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/fmt/os.h b/include/fmt/os.h index 01a31bafe105..58a16a5706c5 100644 --- a/include/fmt/os.h +++ b/include/fmt/os.h @@ -385,7 +385,7 @@ class file_buffer final : public detail::buffer { FMT_API void grow(size_t) override; public: - FMT_API file_buffer(cstring_view path, const detail::ostream_params& params); + FMT_API file_buffer(cstring_view path, const ostream_params& params); FMT_API file_buffer(file_buffer&& other); FMT_API ~file_buffer(); @@ -408,7 +408,7 @@ FMT_END_DETAIL_NAMESPACE constexpr detail::buffer_size buffer_size{}; /** A fast output stream which is not thread-safe. */ -class FMT_API ostream final { +class FMT_API ostream { private: FMT_MSC_WARNING(suppress : 4251) detail::file_buffer buffer_; From 7fa287db73f08d8a078fc75821a7912968e0c49c Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Sun, 23 Oct 2022 00:36:53 +0300 Subject: [PATCH 6/6] Apply suggestions from code review --- include/fmt/os.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/fmt/os.h b/include/fmt/os.h index 58a16a5706c5..d94075a8fe5f 100644 --- a/include/fmt/os.h +++ b/include/fmt/os.h @@ -379,7 +379,7 @@ struct ostream_params { # endif }; -class file_buffer final : public detail::buffer { +class file_buffer final : public buffer { file file_; FMT_API void grow(size_t) override;