Skip to main content
added 518 characters in body
Source Link
Loki Astari
  • 96.7k
  • 5
  • 125
  • 338

The value returned by sync is incorrect.

According to the specs:

Returns zero, which indicates success. A value of -1 would indicate failure.

On failure you return EOF. Yes I get that it is -1. But your making assumptions. Follow specs to the letter.

Things like this scare me:

I have been bitten a couple of times were macros expand to more than a single statement. Which renders the block incorrect. As a result I always use '{}' for sub blocks. I recommend it for everything it will prevent those impossible to find errors.

Since we now live in 2014 (nearly 2015). You should be using the modern version of the compiler. So add override to functions that override virtual functions.

int overflow(int ch = EOF)  override;
int sync()                  override;

Definitely worth overriding the method streamsize xsputn (const char* s, streamsize n);

Things like this scare me:

I have been bitten a couple of times were macros expand to more than a single statement. Which renders the block incorrect. As a result I always use '{}' for sub blocks. I recommend it for everything it will prevent those impossible to find errors.

The value returned by sync is incorrect.

According to the specs:

Returns zero, which indicates success. A value of -1 would indicate failure.

On failure you return EOF. Yes I get that it is -1. But your making assumptions. Follow specs to the letter.

Things like this scare me:

I have been bitten a couple of times were macros expand to more than a single statement. Which renders the block incorrect. As a result I always use '{}' for sub blocks. I recommend it for everything it will prevent those impossible to find errors.

Since we now live in 2014 (nearly 2015). You should be using the modern version of the compiler. So add override to functions that override virtual functions.

int overflow(int ch = EOF)  override;
int sync()                  override;

Definitely worth overriding the method streamsize xsputn (const char* s, streamsize n);

added 795 characters in body
Source Link
Loki Astari
  • 96.7k
  • 5
  • 125
  • 338
std::size_t written = 0;
std::size_t size    = pptr() - pbase();

while(written != size)
{
    std::size output = write(fp_, buffer_ + written, size - written);
    if (output == -1)
    {
        if (errno == EINTR || errno == EAGAIN)
        {
             continue; // Ignore the error and try again.
        }
        // Something bad happened.
        abort(1); // or throwset the state of your stream to bad in
                  // some way (I used abort() simply because I am 
                  // to lazy to look up what to do. (you should).
    }
    written += output;
}

The value returned by overflow is incorrect.

According to the specs:

In case of success, the character put is returned, converted to a value of type int_type using member traits_type::to_int_type. Otherwise, it returns the end-of-file value (EOF) either if called with this value as argument c or to signal a failure (some implementations may throw an exception instead).

On success you return 0.

Things like this scare me:

  else
    return EOF; // return EOF on device failure

I have been bitten a couple of times were macros expand to more than a single statement. Which renders the block incorrect. As a result I always use '{}' for sub blocks. I recommend it for everything it will prevent those impossible to find errors.

std::size_t written = 0;
std::size_t size    = pptr() - pbase();

while(written != size)
{
    std::size output = write(fp_, buffer_ + written, size - written);
    if (output == -1)
    {
        if (errno == EINTR || errno == EAGAIN)
        {
             continue; // Ignore the error and try again.
        }
        // Something bad happened.
        abort(1); // or throw
    }
    written += output;
}
std::size_t written = 0;
std::size_t size    = pptr() - pbase();

while(written != size)
{
    std::size output = write(fp_, buffer_ + written, size - written);
    if (output == -1)
    {
        if (errno == EINTR || errno == EAGAIN)
        {
             continue; // Ignore the error and try again.
        }
        // Something bad happened.
        abort(1); // or set the state of your stream to bad in
                  // some way (I used abort() simply because I am 
                  // to lazy to look up what to do. (you should).
    }
    written += output;
}

The value returned by overflow is incorrect.

According to the specs:

In case of success, the character put is returned, converted to a value of type int_type using member traits_type::to_int_type. Otherwise, it returns the end-of-file value (EOF) either if called with this value as argument c or to signal a failure (some implementations may throw an exception instead).

On success you return 0.

Things like this scare me:

  else
    return EOF; // return EOF on device failure

I have been bitten a couple of times were macros expand to more than a single statement. Which renders the block incorrect. As a result I always use '{}' for sub blocks. I recommend it for everything it will prevent those impossible to find errors.

added 795 characters in body
Source Link
Loki Astari
  • 96.7k
  • 5
  • 125
  • 338

You should not do this:

class device_ostream : public std::ostream {
public:
  device_ostream() : std::ostream(new device_buffer) {}

  ~device_ostream() {
    delete rdbuf();
  }
};

You do not know if the user of the class has reset the buffer while you were not watching.

device_ostream   data;
....
MyImprovedStreamBuffer   myBuffer;
data.rdbuf(&myBuffer);               // Your destructor will not play well with that.

I would make it an internal member.

class device_ostream : public std::ostream
{
    device_buffer   buffer;
  public:
    device_ostream()
        : std::ostream(nullptr)
    {
        rdbuf(&buffer);
    }
};

You should validate the return code of C functions.

fwrite(buffer_, 1, size_t(pptr() - pbase()), fp_);

Does not guarantee that it will write everything you asked for. Personally I would use a lower level write function so I can accurately check the error state and compensate accordingly.

std::size_t written = 0;
std::size_t size    = pptr() - pbase();

while(written != size)
{
    std::size output = write(fp_, buffer_ + written, size - written);
    if (output == -1)
    {
        if (errno == EINTR || errno == EAGAIN)
        {
             continue; // Ignore the error and try again.
        }
        // Something bad happened.
        abort(1); // or throw
    }
    written += output;
}

You should not do this:

class device_ostream : public std::ostream {
public:
  device_ostream() : std::ostream(new device_buffer) {}

  ~device_ostream() {
    delete rdbuf();
  }
};

You do not know if the user of the class has reset the buffer while you were not watching.

device_ostream   data;
....
MyImprovedStreamBuffer   myBuffer;
data.rdbuf(&myBuffer);               // Your destructor will not play well with that.

I would make it an internal member.

class device_ostream : public std::ostream
{
    device_buffer   buffer;
  public:
    device_ostream()
        : std::ostream(nullptr)
    {
        rdbuf(&buffer);
    }
};

You should not do this:

class device_ostream : public std::ostream {
public:
  device_ostream() : std::ostream(new device_buffer) {}

  ~device_ostream() {
    delete rdbuf();
  }
};

You do not know if the user of the class has reset the buffer while you were not watching.

device_ostream   data;
....
MyImprovedStreamBuffer   myBuffer;
data.rdbuf(&myBuffer);               // Your destructor will not play well with that.

I would make it an internal member.

class device_ostream : public std::ostream
{
    device_buffer   buffer;
  public:
    device_ostream()
        : std::ostream(nullptr)
    {
        rdbuf(&buffer);
    }
};

You should validate the return code of C functions.

fwrite(buffer_, 1, size_t(pptr() - pbase()), fp_);

Does not guarantee that it will write everything you asked for. Personally I would use a lower level write function so I can accurately check the error state and compensate accordingly.

std::size_t written = 0;
std::size_t size    = pptr() - pbase();

while(written != size)
{
    std::size output = write(fp_, buffer_ + written, size - written);
    if (output == -1)
    {
        if (errno == EINTR || errno == EAGAIN)
        {
             continue; // Ignore the error and try again.
        }
        // Something bad happened.
        abort(1); // or throw
    }
    written += output;
}
Source Link
Loki Astari
  • 96.7k
  • 5
  • 125
  • 338
Loading