2016-07-07 1:22 GMT+03:00 Mike Gelfand <mike...@mikedld.com>:
>
> On 07/06/2016 10:12 PM, Dāvis Mosāns wrote:
> > --- a/Source/kwsys/FStream.hxx.in
> > +++ b/Source/kwsys/FStream.hxx.in
> > @@ -14,33 +14,76 @@
> >
> > #include <@KWSYS_NAMESPACE@/Encoding.hxx>
> > #include <fstream>
> > +#if defined(_WIN32) && !defined(_MSC_VER)
> > +#include <ext/stdio_filebuf.h>
> > +#endif
> >
> > namespace @KWSYS_NAMESPACE@
> > {
> AFAIK, libc++ doesn't have this header, but only libstdc++ does. Not
> sure if Clang on Windows defines _MSC_VER (the one included with recent
> MSVS versions might, but standalone one is another story), might be
> better to check for HAVE_EXT_STDIO_FILEBUF_H macro instead (or in
> addition), to be defined externally on configuration phase. Same goes
> for a few checks down in the same file.
>
I currently don't have other compilers besides MSVC (2015) and MinGW
(GCC 6.1.1) so I tested only on those. I don't know what Clang or Cygwin
would use.
> > @@ -56,7 +99,24 @@ namespace @KWSYS_NAMESPACE@
> > }
> > void open(char const *file_name,std::ios_base::openmode mode =
> > std::ios_base::in)
> > {
> > - if(!buf_->open(file_name,mode | std::ios_base::in))
> > + mode = mode | std::ios_base::in;
> > +#if defined(_MSC_VER)
> > + const bool success = buf_->open(file_name,mode) != 0;
> > +#else
> > + const std::wstring wstr = Encoding::ToWide(file_name);
> > + bool success = false;
> > + std::wstring cmode = getcmode(mode);
> > + file_ = _wfopen(wstr.c_str(), cmode.c_str());
> > + if (file_) {
> > + if (buf_) {
> > + delete buf_;
> > + }
> > + buf_ = new internal_buffer_type(file_, mode);
> > + this->set_rdbuf(buf_);
> > + success = true;
> > + }
> > +#endif
> > + if(!success)
> > {
> > this->setstate(std::ios_base::failbit);
> > }
> Looks odd that you check whether `buf_` is set before assigning, but
> don't check if `file_` is. Either one check is missing or another is
> redundant; the former is more probable since per STL documentation (e.g.
> http://www.cplusplus.com/reference/fstream/fstream/open/), "If the
> stream is already associated with a file (i.e., it is already open),
> calling this function fails."
Indeed I forgot about case when stream is already open. Need to return
early from that. And yeah I guess can delete buf_ unconditionally.
>
> > @@ -75,7 +135,14 @@ namespace @KWSYS_NAMESPACE@
> > }
> > void close()
> > {
> > - if(!buf_->close())
> > + bool success = buf_->close() != 0;
> > +#if !defined(_MSC_VER)
> > + if (file_) {
> > + success = fclose(file_) == 0 ? success : false;
> > + file_ = 0;
> > + }
> > +#endif
> > + if(!success)
> > {
> > this->setstate(std::ios_base::failbit);
> > }
> `success` is overwritten inside newly added condition w/o checking
> previous value; looks odd as well.
>
Look more carefully.
success = fclose(file_) == 0 ? success : false;
We return success only if both buf_->close() and fclose(file_)
returned success. If any of them failed we return failure.
>
> > @@ -92,19 +159,26 @@ namespace @KWSYS_NAMESPACE@
> > [snip]
> > private:
> > internal_buffer_type* buf_;
> > +#if !defined(_MSC_VER)
> > + FILE *file_ = 0;
> > +#endif
> > };
> >
> > template<typename CharType,typename Traits = std::char_traits<CharType> >
> In-place member initialization... As Brad wrote earlier, using C++11 in
> KWSys is not advised.
>
This was already there but only used for _MSC_VER >= 1400
Maybe could just guard this with __cplusplus >= 201103L and not support
Unicode for older, it would be much easier...
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
