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

[Mike Gelfand]

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.

> +  inline const std::wstring getcmode(const std::ios_base::openmode mode) {
Const on return value makes no sense. Some compilers may even warn about it.

> @@ -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."

> @@ -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.

> @@ -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.

> @@ -119,7 +193,24 @@ class basic_ofstream : public std::basic_ostream<CharType,Traits>
>    }
>    void open(char const *file_name,std::ios_base::openmode mode = std::ios_base::out)
>    {
> -    if(!buf_->open(file_name,mode | std::ios_base::out))
> +    mode = mode | std::ios_base::out;
> +#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);
>      }
Mostly duplicate from istream, consider factoring out in some way.

Regards,
Mike