Opened 6 months ago

Closed 6 months ago

#16018 closed enhancement (fixed)

wxFile::Read shows debug message when pBuf == NULL even if nCount == 0

Reported by: trivia21 Owned by:
Priority: low Milestone:
Component: base Version: dev-latest
Keywords: Cc:
Blocked By: Blocking:
Patch: yes

Description

file.Read(NULL, 0) displays a debug message saying pBuf == NULL. I think it should work with any pointer if nCount == 0, because this way it would be easier to write "generic" code that works just the same with all nCount values. Now you have to check for nCount == 0 first each time it isn't hard-coded.
An example of this usage: it's quite reasonable from a MemoryBuffer object to return NULL if I request a buffer size of 0. Now it would be nice if I could use file.Read(membuf.GetBuffer(size), size), and didn't have to check the value of 'size' first.

Attachments (2)

fileread.patch download (420 bytes) - added by trivia21 6 months ago.
patch
filereadwrite.patch download (673 bytes) - added by trivia21 6 months ago.
Same applies to wxFile::Write

Download all attachments as: .zip

Change History (7)

Changed 6 months ago by trivia21

patch

Changed 6 months ago by trivia21

Same applies to wxFile::Write

comment:1 Changed 6 months ago by vadz

If we do this here, we should also do it for wxFFile for consistency. But to be honest I'm not really sure if it's worth it, it doesn't seem to make much sense to allow this expensive (it involves a syscall!) NOP. Well, this could be addressed by just returning 0 immediately, i.e. check for this even before checking for valid buffer, but then we complicate the code by a very, very rare use case...

So while it doesn't harm anybody, I'm not sure if it really helps neither. Any opinions about this?

comment:2 Changed 6 months ago by trivia21

My problem is exactly that it's very rare and unexpected, thus might not occur during testing (even if nCount == 0 is a valid value), then cryptic error messages might start showing up later.
But you are probably right, this additional check would only affect badly optimized code (with potential expensive NOPs), which is not a good reason to complicate code.
How about a wxCHECK version that takes a serious and an acceptable condition, and only shows the debug message when the first is met (but returns from the function either way)?

comment:3 Changed 6 months ago by vadz

I don't think we need a special wxCHECK() version, it'd be enough to just insert if (!count) return; at the beginning of the function. The question is whether it's a good idea or not, i.e. if it's not going to actually hide some bugs.

comment:4 Changed 6 months ago by trivia21

I'm not sure. But if this is the desirable behaviour, please consider adding a warning to the documentation of wxFile::Read and Write stating that pBuf == NULL is illegal and will show a debug message even if nCount == 0.

comment:5 Changed 6 months ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [76030]) Allow passing NULL buffer to wx{,F}File::{Read,Write} when count==0.

If the count of bytes to read or write is 0, the buffer pointer value
shouldn't matter as it's not used at all anyhow, so relax the assert and allow
it to be NULL in this case.

Closes #16018.

Note: See TracTickets for help on using tickets.