custom win32_file_streambuf

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

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 pbump(-static_cast(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.

StackExchange Code Review Q#33288, answer score: 5