zturner added inline comments.
================
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:
> petpav01 wrote:
> > 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?
> The released versions of lldb - at least the ones Apple releases - have
> asserts disabled. This isn't unique to lldb, clang does the same thing.
>
> I do my day-to-day debugging using a TOT build with asserts enabled, and we
> run the testsuite that way so the asserts catch errors at this stage. But
> for the general public, the function will behave as described. It would be
> great to remove the duplicated docs - that's just begging for one or the
> other to get out of date. But the descriptions are functionally correct.
> And then changing the text to "invalid byte size" also seems good to me.
Being pedantic, this *is* a functionality change. Previously, we would assert
on a size of 3 or 0, with this change we will allow those cases through.
https://reviews.llvm.org/D38394
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
