[haiku-commits] Re: r40800 - haiku/trunk/src/add-ons/translators/raw

  • From: Ryan Leavengood <leavengood@xxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Fri, 4 Mar 2011 07:46:38 -0500

On Fri, Mar 4, 2011 at 2:53 AM, Ingo Weinhold <ingo_weinhold@xxxxxx> wrote:
>
> The code is flawed in several ways:
>
> * Mostly aesthetical: When people write ">= B_OK" they usually mean ">= 0".
> Functions like this usually return either a value >= 0 on success or an
> error code on failure. B_OK is never returned, though.

OK, but this problem more or less existed in the code before. Which
probably isn't a great excuse...

> * fError is a status_t. It should not be assigned a ssize_t value. This is
> not only for aesthetical reasons: On 64 bit architectures ssize_t is
> actually wider than status_t.

Again, a problem which existed before. I suppose you have never looked
at this code before (me neither.)

> * Obviously the code expects size(T) bytes of data. The case that less than
> that is read is silently ignored and the caller is left with partial (or
> since this change even) complete garbage.

Before this change a particular JPEG file could not be opened and
after it can. So reading 0 bytes can happen in real life.

Given your concern over mixing status_t and ssize_t probably most of
the methods of this class need to be changed not to assign the result
of reading the stream to fError. So probably something like:

template<class T> inline void
operator()(T &data)
{
        ssize_t bytesRead = fStream.Read((void *)&data, sizeof(T));
        if (bytesRead > 0) {
                if (IsSwapping())
                        byte_swap(data);
                return;
        }

        if (bytesRead == 0)
                fError = B_ERROR;
        throw fError;
}

Then clients of this class need to catch that throw and deal with it
however is appropriate. I can dig into it more to see why the test
JPEG causes this problem.

> * The Next() method duplicates the code (it should call this operator
> instead) and still has the old comparison.

Good point. Most of the other methods would need to be changed to be
more like the above example.

-- 
Regards,
Ryan

Other related posts: