[cmake-developers] [PATCH v3 7/7] Add MinGW support for FStream

[Dāvis Mosāns]

2016-07-07 1:22 GMT+03:00 Mike Gelfand <mikedld at 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...