6
\$\begingroup\$

I am looking for a review regarding C++ streams behaviour conformance.

I have made this win32_file_streambuf so that I can use it in log4cplus project. I basically need it so that log files can be renamed when they are still opened by another process logging into the same file. This can be done when file is opened with FILE_SHARE_DELETE share mode flag.

The win32_file_streambuf has its limitations, like not being able to seek to arbitrary offset in the file when its encoding is variable width.

According to some people, it is better to use review feature on BitBucket.org.

// File:    custom_ostream.cpp
// Created: 9/2013
// Author:  Vaclav Zeman
//
//
//  Copyright (C) 2013, Vaclav Zeman. All rights reserved.
//  
//  Redistribution and use in source and binary forms, with or without modifica-
//  tion, are permitted provided that the following conditions are met:
//  
//  1. Redistributions of  source code must  retain the above copyright  notice,
//     this list of conditions and the following disclaimer.
//  
//  2. Redistributions in binary form must reproduce the above copyright notice,
//     this list of conditions and the following disclaimer in the documentation
//     and/or other materials provided with the distribution.
//  
//  THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESSED OR IMPLIED WARRANTIES,
//  INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
//  FITNESS  FOR A PARTICULAR  PURPOSE ARE  DISCLAIMED.  IN NO  EVENT SHALL  THE
//  APACHE SOFTWARE  FOUNDATION  OR ITS CONTRIBUTORS  BE LIABLE FOR  ANY DIRECT,
//  INDIRECT, INCIDENTAL, SPECIAL,  EXEMPLARY, OR CONSEQUENTIAL  DAMAGES (INCLU-
//  DING, BUT NOT LIMITED TO, PROCUREMENT  OF SUBSTITUTE GOODS OR SERVICES; LOSS
//  OF USE, DATA, OR  PROFITS; OR BUSINESS  INTERRUPTION)  HOWEVER CAUSED AND ON
//  ANY  THEORY OF LIABILITY,  WHETHER  IN CONTRACT,  STRICT LIABILITY,  OR TORT
//  (INCLUDING  NEGLIGENCE OR  OTHERWISE) ARISING IN  ANY WAY OUT OF THE  USE OF
//  THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.


//#include "stdafx.h"
#include <ios>
#include <istream>
#include <iostream>
#include <iomanip>
#include <locale>
#include <codecvt>
#include <sstream>
#include <fstream>
#include <string>
#include <vector>
#include <cassert>
#include <limits>
#include <cstdlib>
#include <WinSock2.h>


void
clear_mbstate (std::mbstate_t & mbs)
{
    // Initialize/clear mbstate_t type.
    // XXX: This is just a hack that works. The shape of mbstate_t varies
    // from single unsigned to char[128]. Without some sort of initialization
    // the codecvt::in/out methods randomly fail because the initial state is
    // random/invalid.
    std::memset (&mbs, 0, sizeof (std::mbstate_t));
}


template <typename OuterRep, typename InnerChar>
std::codecvt_base::result
do_codecvt_out (std::vector<OuterRep> & dest, InnerChar const * src,
    std::size_t size, std::ptrdiff_t & converted,
    std::codecvt<InnerChar, OuterRep, std::mbstate_t> const & cdcvt,
    std::mbstate_t & state)
{
    if (size == 0)
    {
        dest.clear ();
        return std::codecvt_base::noconv;
    }

    InnerChar const * from_first = src;
    std::size_t const from_size = size;
    InnerChar const * const from_last = from_first + from_size;
    InnerChar const * from_next = from_first;

    // If the destination is too small here and the facet is codecvt_utf8
    // facet, then the conversion will fail because there is not enough space
    // for all bytes of the output. IMHO, the codecvt_utf8 facet should be
    // handling the overflow through the std::mbstate_t parameter but
    // unfortunately it does not do that.
    dest.resize ((std::max) (from_size,
        static_cast<std::size_t>(cdcvt.max_length ())));

    OuterRep * to_first = &dest.front ();
    std::size_t to_size = dest.size ();
    OuterRep * to_last = to_first + to_size;
    OuterRep * to_next = to_first;

    std::codecvt_base::result result;
    std::size_t converted_out = 0;
    while (from_next != from_last)
    {
        result = cdcvt.out (
            state, from_first, from_last,
            from_next, to_first, to_last,
            to_next);
        // XXX: Even if only half of the input has been converted the
        // in() method returns CodeCvt::ok with VC8. I think it should
        // return CodeCvt::partial.
        if (result == std::codecvt_base::ok
            && from_next != from_last)
        {
            to_size = dest.size () * 2;
            dest.resize (to_size);
            converted_out = to_next - to_first;
            to_first = &dest.front ();
            to_last = to_first + to_size;
            to_next = to_first + converted_out;
        }
        else
            break;
    }
    converted_out = to_next - &dest[0];
    dest.resize(converted_out);
    converted = from_next - from_first;

    return result;
}


template <typename Char>
struct win32_file_ops;


template <>
struct win32_file_ops<char>
{
    static
    HANDLE
    create (char const * file_name, DWORD desired_access,
        DWORD share_mode, DWORD creation_disposition,
        DWORD flags_and_attributes)
    {
        return CreateFileA (file_name, desired_access, share_mode, 0,
            creation_disposition, flags_and_attributes, 0);
    }
};


template <>
struct win32_file_ops<wchar_t>
{
    static
    HANDLE
    create (wchar_t const * file_name, DWORD desired_access,
        DWORD share_mode, DWORD creation_disposition,
        DWORD flags_and_attributes)
    {
        return CreateFileW (file_name, desired_access, share_mode, 0,
            creation_disposition, flags_and_attributes, 0);
    }
};


template <typename Char, typename Traits = std::char_traits<Char> >
class win32_file_streambuf
    : public std::basic_streambuf<Char, Traits>
{
    enum constants
    {
        DATA_BUF_INITIAL_SIZE = 1024 * 64 / sizeof (Char)
    };

public:
    typedef std::basic_streambuf<Char, Traits> base_type;
    typedef typename base_type::char_type char_type;
    typedef typename base_type::int_type int_type;
    typedef typename base_type::traits_type traits_type;
    typedef typename base_type::pos_type pos_type;
    typedef std::ios_base::openmode openmode;
    typedef typename traits_type::off_type off_type;

    win32_file_streambuf ()
        : fh (INVALID_HANDLE_VALUE)
        , data_buf ()
        , loc ()
        , cdcvt (0)
        , file_pos (0)
        , open_mode (openmode (0))
    {
        imbue (loc);
        this->setp (0, 0, 0);
    }


    virtual ~win32_file_streambuf ()
    {
        try
        {
            close ();
        }
        catch (std::ios_base::failure const &)
        { }
    }


protected:
    static
    void
    prepare_flags(DWORD & desired_access, DWORD & share_mode,
        DWORD & creation_disposition, DWORD & flags_and_attributes,
        openmode om)
    {
        desired_access = FILE_GENERIC_READ | STANDARD_RIGHTS_WRITE
            | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_WRITE_DATA;
        share_mode = FILE_SHARE_READ | FILE_SHARE_WRITE
            | FILE_SHARE_DELETE;
        creation_disposition = 0;

        if (om & std::ios_base::trunc)
            creation_disposition |= CREATE_ALWAYS;
        else
            creation_disposition |= OPEN_ALWAYS;

        flags_and_attributes = FILE_ATTRIBUTE_ARCHIVE;
    }


    template <typename FNChar>
    win32_file_streambuf *
    do_open (FNChar const * file_name, openmode om)
    {
        open_mode = om;

        DWORD desired_access, share_mode, creation_disposition,
            flags_and_attributes;
        prepare_flags (desired_access, share_mode, creation_disposition,
            flags_and_attributes, open_mode);

        fh = win32_file_ops<FNChar>::create (file_name, desired_access,
            share_mode, creation_disposition, flags_and_attributes);

        if (! fh
            || fh == INVALID_HANDLE_VALUE)
            return 0;

        if (open_mode & std::ios_base::ate
            && do_seekoff (0, std::ios_base::end) == off_type (-1))
            return 0;

        file_pos = win32_get_file_pos ();

        return this;
    }


public:
    win32_file_streambuf *
    open(wchar_t const * file_name, openmode om)
    {
        return do_open (file_name, om);
    }


    win32_file_streambuf *
    open(char const * file_name, openmode om)
    {
        return do_open (file_name, om);
    }


    win32_file_streambuf *
    close ()
    {
        int_type res
            = sync ()
            || flush_conversion_state ();

        char_type * pb = this->pbase ();
        this->setp (pb, pb, this->epptr ());

        if (fh != INVALID_HANDLE_VALUE)
        {
            BOOL ret = CloseHandle (fh);
            if (! ret)
                //throw std::ios_base::failure ("CloseHandle");
                return 0;

            fh = INVALID_HANDLE_VALUE;
        }

        return res == 0 ? this : 0;
    }


    bool
    is_open () const
    {
        return fh && fh != INVALID_HANDLE_VALUE;
    }


protected:
    static
    bool
    is_eof (int_type ch)
    {
        return traits_type::eq_int_type(ch, traits_type::eof ());
    }

    static
    int_type
    not_eof ()
    {
        return traits_type::not_eof(traits_type::eof ());
    }


    off_type
    win32_get_file_pos ()
    {
        LARGE_INTEGER to = { };
        LARGE_INTEGER cur = { };
        BOOL ret = SetFilePointerEx (fh, to, &cur, FILE_CURRENT);
        if (! ret)
            return off_type (-1);
        else
            return off_type (cur.QuadPart);
    }


    off_type
    win32_get_file_size ()
    {
        LARGE_INTEGER li;
        BOOL ret = GetFileSizeEx (fh, &li);
        if (! ret
            || (std::numeric_limits<off_type>::max) () < li.QuadPart)
            return off_type (-1);
        else
            return off_type (li.QuadPart);
    }


    BOOL
    win32_write_file (char const * buf, DWORD to_write, DWORD & written)
    {
        // Perepare OVERLAPPED structure.

        OVERLAPPED overlapped = { };
        if (open_mode & std::ios_base::app)
        {
            overlapped.OffsetHigh = 0xFFFFFFFFu;
            overlapped.Offset = 0xFFFFFFFFu;
        }
        else
        {
            overlapped.OffsetHigh = LONGLONG (file_pos) >> 32;
            overlapped.Offset = LONGLONG (file_pos) & 0xFFFFFFFFu;
        }

        // Do the actual write.

        BOOL wfret = WriteFile (fh, buf, to_write, &written,
            &overlapped);
        return wfret;
    }


    int_type
    update_file_pos (DWORD written)
    {
        if (open_mode & std::ios_base::app)
        {
            off_type file_size = win32_get_file_size ();
            if (file_size == -1)
                return traits_type::eof ();
        }
        else
            file_pos += written;

        return not_eof ();
    }


    virtual
    int_type
    overflow (int_type ch)
    {
        int_type ret = traits_type::not_eof (ch);

        // Set up a buffer if we do not have one, yet.

        if (! this->pbase())
        {
            data_buf.resize(DATA_BUF_INITIAL_SIZE);
            this->setp(&data_buf[0], &data_buf[0],
                &data_buf[0] + data_buf.size());
        }

        for (;;)
        {
            char_type * mid = this->pptr ();       
            std::ptrdiff_t const buffer_space = this->epptr () - mid;

            // Try to store a character if it is not EOF
            // and we have a space for it.

            if (! is_eof (ch)
                && buffer_space > 0)
            {
                *mid = traits_type::to_char_type (ch);
                this->pbump (1);                
                break;
            }

            // Check for anything to do first.

            else if (mid == this->pbase ())
            {
                // Nothing to do.
                break;
            }

            // Else flush buffer.

            else
            {
                char_type * const pb = this->pbase ();

                std::size_t to_write = 0;
                char const * buf = 0;
                char_type * new_pmid = 0;

                // Do characters need a conversion?

                if (cdcvt->always_noconv ())
                {
                    // No conversion is necessary.

                    buf = reinterpret_cast<char const *>(pb);
                    to_write = (mid - pb) * sizeof (char_type);
                    new_pmid = pb;
                }
                else
                {
                    // Convert using codecvt facet from our locale.

                    std::ptrdiff_t converted = 0;
                    std::ptrdiff_t const to_convert = mid - pb;
                    std::codecvt_base::result codecvt_res
                        = do_codecvt_out<char> (out_buf, pb, to_convert,
                            converted, *cdcvt, cdcvt_state);

                    if (codecvt_res == std::codecvt_base::noconv)
                    {
                        buf = reinterpret_cast<char const *>(pb);
                        to_write = to_convert;
                        new_pmid = pb;
                    }
                    else if (codecvt_res == std::codecvt_base::error)
                    {
                        ret = traits_type::eof ();
                        break;
                    }
                    else
                    {                        
                        if (converted < to_convert)
                        {
                            // Handle partial conversion.
                            // Move remaining characters down.

                            std::ptrdiff_t const remaining_conv
                                = to_convert - converted;
                            traits_type::move (pb, pb + converted,
                                remaining_conv);

                            new_pmid = pb + remaining_conv;
                        }
                        else
                            new_pmid = pb;

                        buf = &out_buf[0];
                        to_write = out_buf.size ();
                    }
                }

                DWORD written = 0;
                BOOL wfret = win32_write_file (buf, to_write, written);
                if (! wfret)
                {
                    //throw std::ios_base::failure("WriteFile");
                    ret = traits_type::eof ();
                    break;
                }

                if (is_eof (update_file_pos (written)))
                {
                    ret = traits_type::eof ();
                    break;
                }

                if (written < to_write)
                {
                    ret = traits_type::eof ();
                    break;
                    //std::ptrdiff_t const remaining = to_write - written;
                    //traits_type::move(pb, pb + written, remaining);
                    //this->pbump(-static_cast<std::ptrdiff_t>(written));
                }

                this->setp (pb, new_pmid, this->epptr ());

                if (! is_eof (ch))
                    // Try character again after we have freed some buffer
                    // space.
                    continue;
                else
                {
                    // Or just exit, if were were just supposed to flush.
                    break;
                }
            }
        }

        return ret;
    }


    virtual
    int sync ()
    {
        if (is_eof (overflow (traits_type::eof ())))
            return -1;
        else
            return 0;
    }


    virtual
    basic_streambuf<Char, Traits> *
    setbuf (char_type * s, std::streamsize n)
    {
        if (! this->pbase () && n > 0)
        {
            this->setp(s, s, s + n);
            return this;
        }
        else
            return 0;
    }


    virtual
    void
    imbue (const std::locale& new_loc)
    {
        loc = new_loc;
        cdcvt = &std::use_facet<codecvt_type>(loc);
        clear_mbstate (cdcvt_state);
    }


private:
    int_type
    flush_conversion_state()
    {
        if (cdcvt->always_noconv())
            return not_eof ();

        char out = 0;
        char * mid = 0;
        std::codecvt_base::result res;
        int_type ret = not_eof ();

        out_buf.resize ((std::max) (static_cast<int>(out_buf.size ()),
            (std::min) (cdcvt->max_length (), 64)));
        for (;;)
        {
            char * const buf = &out_buf[0];
            mid = buf;
            res = cdcvt->unshift (cdcvt_state, buf, buf + out_buf.size (),
                mid);
            if (res == std::codecvt_base::noconv)
                return not_eof ();

            else if (res == std::codecvt_base::partial
                || res == std::codecvt_base::ok)
            {
                DWORD to_write = mid - buf;
                DWORD written = 0;
                BOOL wfret = win32_write_file (buf, to_write, written);
                if (! wfret)
                {
                    //throw std::ios_base::failure("WriteFile");
                    ret = traits_type::eof ();
                    break;
                }

                if (is_eof (update_file_pos (written)))
                {
                    ret = traits_type::eof ();
                    break;
                }

                if (written < to_write)
                {
                    ret = traits_type::eof ();
                    break;
                    //std::ptrdiff_t const remaining = to_write - written;
                    //traits_type::move(pb, pb + written, remaining);
                    //this->pbump(-static_cast<std::ptrdiff_t>(written));
                }

                if (res == std::codecvt_base::partial)
                    continue;
                else
                    break;
            }
            else
            {
                ret = traits_type::eof ();
                break;
            }
        }

        return ret;
    }

    pos_type
    do_seekoff (off_type off, std::ios_base::seekdir dir,
        openmode /* which */ = std::ios_base::in | std::ios_base::out)
    {
        if (! is_open ())
            return pos_type (off_type (-1));

        if (sync () != 0)
            return pos_type (off_type (-1));

        if (is_eof (flush_conversion_state ()))
            return pos_type (off_type (-1));

        int const width = cdcvt->encoding ();
        off_type new_off = 0;
        switch (dir)
        {
        case std::ios_base::beg:
            if (off < 0
                || off != 0 && width <= 0)
                return pos_type (off_type (-1));

            clear_mbstate (cdcvt_state);
            new_off = off * width;
            break;

        case std::ios_base::cur:
            if ((off != 0
                    && width <= 0)
                || (off > 0
                    && (std::numeric_limits<off_type>::max) () - file_pos
                        < off * width)
                || (off < 0
                    && off * width + file_pos < 0))
                return pos_type (off_type (-1));

            new_off = file_pos + off * width;
            break;

        case std::ios_base::end:
            off_type const file_size = win32_get_file_size ();
            if (file_size == off_type (-1))
                return pos_type (file_size);

            if (off < 0
                || (off != 0
                    && width <= 0)
                || off * width > file_size)
                return pos_type (off_type (-1));

            new_off -= off * width;
            break;
        }

        if (new_off == file_pos)
            return file_pos;

        file_pos = new_off;
        return pos_type (file_pos);
    }


protected:
    virtual
    pos_type
    seekpos (pos_type pos, openmode which
        = std::ios_base::in | std::ios_base::out)
    {
        return do_seekoff (pos, std::ios_base::beg, which);
    }


    virtual
    pos_type
    seekoff (off_type off, std::ios_base::seekdir dir,
        openmode which = std::ios_base::in | std::ios_base::out)
    {
        return do_seekoff (off, dir, which);
    }


public:
    std::locale
    getloc () const
    {
        return loc;
    }


private:
    typedef std::codecvt<char_type, char, std::mbstate_t> codecvt_type;

    HANDLE fh;
    std::vector<char_type> data_buf;
    std::vector<char> out_buf;
    std::locale loc;
    codecvt_type const * cdcvt;
    std::mbstate_t cdcvt_state;
    off_type file_pos;
    openmode open_mode;
};


template <typename Char, typename Traits = std::char_traits<Char> >
class win32_file_stream
    : public std::basic_ostream<Char, Traits>
{
public:
    enum { default_open_mode = static_cast<std::ios_base::openmode>(
        std::ios_base::trunc | std::ios_base::out) };


    win32_file_stream ()
        : std::ostream (&sbuf)
    { }


    win32_file_stream (char const * file_name, std::ios_base::openmode om
        = win32_file_stream::default_open_mode)
        : std::basic_ostream<Char, Traits> (&sbuf)
    {
        do_open(file_name, om);
    }


    win32_file_stream (wchar_t const * file_name, std::ios_base::openmode om
        = win32_file_stream::default_open_mode)
        : std::basic_ostream<Char, Traits> (&sbuf)
    {
        do_open(file_name, om);
    }


    win32_file_stream &
    open(char const * file_name, std::ios_base::openmode om
        = win32_file_stream::default_open_mode)
    {
        return do_open (file_name, om);
    }


    win32_file_stream &
    open(wchar_t const * file_name, std::ios_base::openmode om
        = win32_file_stream::default_open_mode)
    {
        return do_open(file_name, om);
    }


    win32_file_stream &
    close ()
    {
        if (! sbuf.close ())
            this->clear (+this->rdstate () | +std::ios_base::badbit);

        this->clear ();
        return *this;
    }


protected:
    template <typename FNChar>
    win32_file_stream &
    do_open (FNChar const * file_name,
        std::ios_base::openmode om)
    {
        if (! sbuf.open(file_name, om))
            this->clear (std::ios_base::badbit);

        return *this;
    }

    win32_file_streambuf<typename char_type> sbuf;
};


template <typename Stream>
void
set_exceptions (Stream & s)
{
    s.exceptions (std::ios_base::failbit | std::ios_base::badbit
        | std::ios_base::eofbit);
}



int main(int argc, char* argv[])
{
#if 0
    std::mbstate_t state = std::mbstate_t ();
    std::locale loc (std::locale (), new std::codecvt_utf8<wchar_t>);
    typedef std::codecvt<wchar_t, char, std::mbstate_t> codecvt_type;
    codecvt_type const & cvt = std::use_facet<codecvt_type> (loc);

    wchar_t ch = L'\u5FC3';
    wchar_t const * from_first = &ch;
    wchar_t const * from_mid = &ch;
    wchar_t const * from_end = from_first + 1;

    char out_buf[1];
    char * out_first = out_buf;
    char * out_mid = out_buf;
    char * out_end = out_buf + 1;

    std::codecvt_base::result cvt_res
        = cvt.out (state, from_first, from_end, from_mid,
            out_first, out_end, out_mid);

    // This is what I expect:
    if (cvt_res == std::codecvt_base::partial
        && out_mid == out_end
        && state != 0)
        ;
    else
        abort ();
#endif

    {
        win32_file_stream<char> fs(L"test.txt");
        set_exceptions (fs);
        fs << "test\n"
           << std::flush;
        fs.close ();
        fs.imbue (std::locale (""));
        fs.open ("test.txt");
        fs << "test\n";
    }

    {
        win32_file_stream<wchar_t> fs("test.txt");
        set_exceptions (fs);
        fs.imbue (std::locale ("Japanese_Japan"));
        fs << L"\u5FC3\n"; // KOKORO
        fs << L"test\n";
        fs.close ();

        std::cerr << "my stream\n";
        fs.open ("test.txt", std::ios_base::app);
        std::cerr << "on open: " << fs.tellp () << "\n";
        fs << L"appended\n";
        std::cerr << "after output: " << fs.tellp () << "\n";
        fs.seekp (0, std::ios_base::beg);
        fs << L"should go to end\n";
    }

    {
        win32_file_stream<wchar_t> fs("test.txt", std::ios_base::app);
        set_exceptions (fs);
        wchar_t buf[1];
        fs.rdbuf ()->pubsetbuf (buf, 1);
        fs.imbue (
            std::locale (
                std::locale (
                    std::locale ("Japanese_Japan"),
                    new std::codecvt_utf8<wchar_t>),
                new std::codecvt_utf8<char>));
        fs << L"\u5FC4\n";
    }

    {
        std::cerr << "ofstream\n";
        std::ofstream ofs ("test.txt", std::ios_base::app);
        set_exceptions (ofs);
        std::cerr << "on open: " << ofs.tellp () << "\n";
        ofs << "from ofstream\n";
        std::cerr << "after output: " << ofs.tellp () << "\n";
    }

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Be careful imbuing a file after it has been opened. Once any data is read from the stream an imbue will usually fail. Sometimes when a file is opened the stream will read the BOM marker (thus implicitly reading from the stream). This read of the BOM can potentially cause an subsequent imbue to fail. Best to declare/imbue/open. \$\endgroup\$ – Martin York Oct 26 '13 at 13:33
  • 4
    \$\begingroup\$ Lots of good looking code here. I will try and give it a bash over the weekend. But this site has very few good C++ reviewers (we are mostly used to seeing bad C++ code and correcting the obvious); so you may want to get it reviewed by other sources as well. \$\endgroup\$ – Martin York Oct 26 '13 at 13:35
5
\$\begingroup\$

When you have an if/else statement, and you find it necessary to use curly braces on the if statement, you should also use curly braces on the else statement.

Like here:

    if (result == std::codecvt_base::ok
        && from_next != from_last)
    {
        to_size = dest.size () * 2;
        dest.resize (to_size);
        converted_out = to_next - to_first;
        to_first = &dest.front ();
        to_last = to_first + to_size;
        to_next = to_first + converted_out;
    }
    else
        break;

you should have encompassed the else in curly brace, or even better you should have made this a guard clause to break from the while.

Like this:

if (result != std::codecvt_base::ok
    && from_next == from_last)
{
    break;
}
to_size = dest.size () * 2;
dest.resize (to_size);
converted_out = to_next - to_first;
to_first = &dest.front ();
to_last = to_first + to_size;
to_next = to_first + converted_out;

Here you have 2 specific instances where you want the function to return 0;:

    if (! fh
        || fh == INVALID_HANDLE_VALUE)
        return 0;

    if (open_mode & std::ios_base::ate
        && do_seekoff (0, std::ios_base::end) == off_type (-1))
        return 0;

I think that you should merge these into one if statement, but if you just merge the conditional statement it will look messy. You should find good variable names for each condition statement and use the variables in one if statement, something like this:

bool firstCondition = ! fh || fh = INVALID_HANDLE_VALUE;
bool secondCondition = open_mode & std::ios_base::ate && do_seekoff (0, std::ios_base::end) == off_type (-1);

if (firstCondition || secondCondition)
{
    return 0;
}

Here again you have an if/else statement where you use curly braces inconsistently:

int_type
update_file_pos (DWORD written)
{
    if (open_mode & std::ios_base::app)
    {
        off_type file_size = win32_get_file_size ();
        if (file_size == -1)
            return traits_type::eof ();
    }
    else
        file_pos += written;

    return not_eof ();
}

I advise against this. I think that when you didn't use curly braces inside the if block, on the one lined if statement, that it is acceptable, I personally don't like it but it seems reasonable. The else statement should have braces because the accompanying if statement needs the braces.


I stopped at this else statement:

else
{                        
    if (converted < to_convert)
    {
        // Handle partial conversion.
        // Move remaining characters down.

        std::ptrdiff_t const remaining_conv
            = to_convert - converted;
        traits_type::move (pb, pb + converted,
            remaining_conv);

        new_pmid = pb + remaining_conv;
    }
    else
        new_pmid = pb;

    buf = &out_buf[0];
    to_write = out_buf.size ();
}

My first thought was that this should just be another else if then else statement instead of a nested if/else statement, but then I noticed that you did that fun stuff with the curly braces again, and that there are 2 lines of code that need to run if the else statement is hit no matter what the nested if statement calculates to.

Again I am going to say that I think you should use the curly braces on the else statement because you used them on the if statement.


In the same long if/else if/else statement nest you have the following:

DWORD written = 0;
BOOL wfret = win32_write_file (buf, to_write, written);
if (! wfret)
{
    //throw std::ios_base::failure("WriteFile");
    ret = traits_type::eof ();
    break;
}

if (is_eof (update_file_pos (written)))
{
    ret = traits_type::eof ();
    break;
}

if (written < to_write)
{
    ret = traits_type::eof ();
    break;
    //std::ptrdiff_t const remaining = to_write - written;
    //traits_type::move(pb, pb + written, remaining);
    //this->pbump(-static_cast<std::ptrdiff_t>(written));
}

I really think that you should merge these together using the || functionality in your conditional, this will reduce these 3 statements down to just 1 statement.

Ending up with this:

DWORD written = 0;
BOOL wfret = win32_write_file (buf, to_write, written);
if ((! wfret) || (is_eof (update_file_pos (written))) || (written < to_write))
{
    ret = traits_type::eof ();
    break;
}

Note: I ignored the comments in these statements.

Copy Paste Alert

In the next private block I found that you have the same exact 3 if statements preceded by 2 variables define the same exact way as above. Maybe you should think about making this a function and pass in the parameters?


Another 3 if statements that should be merged using the || operator:

if (! is_open ())
    return pos_type (off_type (-1));

if (sync () != 0)
    return pos_type (off_type (-1));

if (is_eof (flush_conversion_state ()))
    return pos_type (off_type (-1));
if ((! is_open ()) || (sync () != 0) || (is_eof (flush_conversion_state ())))
    return pos_type (off_type (-1));

and I am okay with not using the curly braces here, although I prefer to use them all the time.

\$\endgroup\$
  • 1
    \$\begingroup\$ You should find good variable names for each condition statement and use the variables in one if statement, - why didn't you propose such names? Your proposed solution, as it stands, is certainly worse than the original. Besides, you haven't explained why merging the statements is a good idea. \$\endgroup\$ – Bartek Banachewicz Apr 20 '15 at 14:09
  • \$\begingroup\$ I am not familiar with C++ naming standards, and I thought it was pretty clear that one if statement was better than 2? please do correct me if I am wrong in that assumption, @BartekBanachewicz. \$\endgroup\$ – Malachi Apr 20 '15 at 14:12
  • \$\begingroup\$ I don't like that this answer suggests that there are times when curly braces might be unnecessary for if statements, but I upvoted anyway... \$\endgroup\$ – nhgrif Apr 20 '15 at 14:13
  • 2
    \$\begingroup\$ This is all about formatting. It's trivial. Why not give a useful review about the code's functionality and design? I must say, though, I agree with almost all of your points. \$\endgroup\$ – Lightness Races in Orbit Apr 20 '15 at 14:14
  • 4
    \$\begingroup\$ For those asking why the sudden interest in a 2-year-old post: meta.codereview.stackexchange.com/a/1511/31503 it was a zombie. \$\endgroup\$ – rolfl Apr 20 '15 at 14:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.