Add some unittest on get_buffer #889
Conversation
|
Where is rhe codecov, it seems it has vanished! |
|
I think it doesn't run on drafts |
src/fs_windows.cpp
Outdated
| goto err; | ||
| } | ||
|
|
||
| if (size_read == 0) { |
There was a problem hiding this comment.
Maybe size_read != batch_to_read?
There was a problem hiding this comment.
From ReadFile function, it is technically possible than size_read != batch_to_read. If we reach the end of file or (at least on linux, don't known on windows) if we have been interrupted by a signal.
The eof should not happen as we have check validity of the read before.
But I prefer to keep it. If it appear it is really something wrong (unexpected eof), we would have a step more in the loop before throwing a error (and mostly stopping the program).
mgautierfr
force-pushed
the
libzim_github_ci_windows
branch
3 times, most recently
from
4d536ce
to
0a1d2c3
Compare
mgautierfr
force-pushed
the
test_reader
branch
2 times, most recently
from
f1ad2e0
to
8f6d1ca
Compare
|
Done |
| virtual void read(char* dest, offset_t offset, zsize_t size) const = 0; | ||
| void read(char* dest, offset_t offset, zsize_t size) const { | ||
| if (can_read(offset, size)) { | ||
| if (size) { | ||
| // Do the actuall read only if we have a size to read | ||
| readImpl(dest, offset, size); | ||
| } | ||
| return; | ||
| } | ||
| throw std::runtime_error("Cannot read after the end of the reader"); | ||
| } |
There was a problem hiding this comment.
It would be nice if the commit message is updated
| zsize_t size() const override; | ||
| offset_t offset() const override; |
There was a problem hiding this comment.
This commit better be moved right after the one introducing Reader::readImpl()
When building with the meson option `b_ndebug=true` (never set on our CI/CD), `ASSERT` macro is removed and so we stop testing that we are actually read in the reader range. Now we introduce `readImpl` which do the actual read (depending of implementation) and make `read` testing read range and call `readImpl` when everything is ok.
Adding few `override` only on `readImpl` now trigger warnings `inconsistent-missing-override`. Better to correctly add `override` when needed.
`FileReader` expects that `readAt` throws an exception if something goes wrong. So `readAt` must throw an exception instead of returning `-1`.
ReadFile take a 32bits size. So we cannot blindly pass a 64bits size. Read by batch of 1GiB.
On Windows `long` is 32bits. It's better to use an explicit size.
Done. Hope picky @veloman-yunkan is happy with the change.
I need a explicit approval to merge. Please approve (and merge directly while you have the button under your mouse cursor) |
This Pr was initially to fix #886. But it appears it was not a bug on our side.
However, it is better testing and it reveals some bug on Windows side.