petpav01 added a comment.
Thank you for the initial review.
================
Comment at: source/Core/DumpDataExtractor.cpp:275-281
+ // Reject invalid item_byte_size.
+ if (item_byte_size > 8) {
+ s->Printf("error: unsupported byte size (%" PRIu64 ") for char format",
+ (uint64_t)item_byte_size);
+ return offset;
+ }
+
----------------
jingham wrote:
> Should this consume the weird input we couldn't print? I actually don't have
> a good feel for which would be better.
The behaviour implemented in `DumpDataExtractor()` for other formats, such as
`eFormatBoolean` or `eFormatComplexInteger`, is to report an error and do not
advance the offset. The approach that the patch takes is to make `eFormatChar`
(and its variants) consistent with this behaviour.
================
Comment at: source/Utility/DataExtractor.cpp:566
size_t byte_size) const {
- switch (byte_size) {
- case 1:
- return GetU8(offset_ptr);
- break;
- case 2:
- return GetU16(offset_ptr);
- break;
- case 4:
- return GetU32(offset_ptr);
- break;
- default:
- assert(false && "GetMaxU32 unhandled case!");
- break;
- }
- return 0;
+ assert(byte_size <= 4 && "GetMaxU32 unhandled case!");
+ return GetMaxU64(offset_ptr, byte_size);
----------------
jingham wrote:
> This is trivial, and you didn't change what was there, but this message makes
> it sound like this is just something we haven't gotten to yet. It's really
> "You passed in an illegal byte size"... Might be clearer if the message said
> that.
I was not sure what is the expected behaviour when the input `byte_size`
exceeds the size of the return type of each of these `GetMax...()` methods. The
current behaviour is to assert this situation but comments describing the
methods (in both `DataExtractor.cpp` and `DataExtractor.h`) say that nothing
should get extracted in these cases and zero is returned.
Maybe the patch should go a bit further and clean this up as follows:
* Remove duplicated comments in `DataExtractor.cpp` for
`DataExtractor::GetMaxU32()` and `GetMaxU64()` and keep only their Doxygen
versions in `DataExtractor.h`.
* Update comments in `DataExtractor.h` for `DataExtractor::GetMaxU32()`,
`GetMaxU64()`, `GetMaxS64()`, `GetMaxU64Bitfield()` and `GetMaxS64Bitfield()`
to match the current implementation.
* Change assertion text in `DataExtractor::GetMaxU32()` and `GetMaxU64()` from
"unhandled case" to "invalid byte size".
Does this sound reasonable?
https://reviews.llvm.org/D38394
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
