-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make fields of DataBuf private #1886
Conversation
32d69f4
to
401bddd
Compare
401bddd
to
c9d0cf3
Compare
Codecov Report
@@ Coverage Diff @@
## main #1886 +/- ##
==========================================
+ Coverage 60.80% 60.86% +0.06%
==========================================
Files 96 96
Lines 18963 19044 +81
Branches 9512 9726 +214
==========================================
+ Hits 11531 11592 +61
- Misses 5131 5139 +8
- Partials 2301 2313 +12
Continue to review full report at Codecov.
|
a6c63e7
to
61a14c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea to change this as part of the new Exiv2/API for v1.00. Several suggestions. It's fine to ignore my suggestions!.
- Provide a default offset=0 for c_str() and c_data().
const byte* c_data(size_t offset=0) const;
-
Is c_data() a good name? Perhaps data(offset=0) if easier on the eye.
-
It there merit in have having
operator =
for common assignments, such as:
unit64_t foo = dataBuf(size_t offset = 0,ByteOrder bo = inherit);
- There is a ByteOrder inherit. It's used by the binaryDecoder magic (and discussed in my book).
I'm wondering if we should have a ByteOrder on the DataBuf constructor.
On a read, the default bo=inherit would mean "use the buffer byteOrder used in the DataBuf constructor." We could avoid lots of ugly byteOrder arguments. For example, when you allocated an IFD, you store the byteOrder in the buffer itself and the code proceeds more smoothly. The byte swapping takes place in the buffer and not in the parser.
@clanmills: Thanks, those are good suggestions.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice contribution done here!
I particularly appreciate the following facts:
- Some reinterpret_cast went away.
- Calls to memset, memcpy, memcmp are now better encapsulated. (Maybe we can get rid of the inclusion of the
cstring
header in some places now).
As Robin mentioned it would be nice to try to improve the byteOrdering issue, but I agree that it would be better to handle it in a separate PR (this one is already quite big).
I made a comment about adding additional unit tests for the new code. But it can be also tackled in a separate PR. Actually, I could offer my help for that 😉
} | ||
|
||
// Test methods like DataBuf::read_uint32 and DataBuf::write_uint32. | ||
TEST(DataBuf, read_write_endianess) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test you are checking the "happy" cases. It would also be good to create additional tests to check what happens on the "non-happy" ones:
- Offset out of limits
- passing null pointers
- etc
It's great to see this merged. Demoting endian handling to the buffer is a different project. |
This a refactoring change to tidy the
DataBuf
class. I made thepData_
andsize_
fields private and added a few new methods which you can use to read and write the data.