asmaloney/libE57Format

Invalid check in CheckedFile.cpp readPhysicalPage

asmaloney opened this issue · 6 comments

In PR #22 - CheckedFile.cpp:

bufView_->read(page_buffer, physicalPageSize);
size_t result = physicalPageSize;

if ( result < 0 || static_cast<size_t>(result) != physicalPageSize )
{
    throw E57_EXCEPTION2(E57_ERROR_READ_FAILED, "fileName=" + fileName_ + " result=" + toString(result));
}

physicalPageSize is a constant, so result isn't a result.

(Also checking a size_t for < 0 is meaningless.)

@jean-noelp What did you mean here?

You're right, this test doesn't make sense anymore.

Since this is a buffer now, reading operation should not be able to fail anyway.

What if the seek right before this code fails?

Then it will set the buffer's cursorStream_ to the size of the buffer and read will start reading there...

If the seek fails, an exception will be throwed in lseek64 at the line 478.

Also the user will have to seek at the beginning, or to close the file.
Normally this is the same behavior as before.

...right. And this is why I hate exceptions :-)

I'll remove that extra code mentioned above.

I understand.
If you want to have explicit exceptions, I suggest you to mention throwable functions like this:
https://en.cppreference.com/w/cpp/language/except_spec

Thanks. I would prefer to go the other way and get rid of them entirely, but I'm not that invested in this code :-)