[vlc-devel] [PATCH] fix qt unsigned comparison warnings

[Pierre Lamot]

Hi, thanks for the v2, I took liberty to update your patch if that's OK 
with you (see enclosed).

* calling place where casting to unsigned which was an issue if the 
index was negative and unchecked.
* I added a prior negative check then did the following tests using 
unsigned (like in your original patch) as most variable are unsigned 
here (i_offset, m_item_list.size(), m_total_count)

Or I can merge your original patch and add the negative check to the 
calling site (as it was already done in *most* place), tough I think 
that the current solution is more fail-proof.

Anyway... I don't intend to waste too much of your time on such details.

Regards

On 2020-09-21 19:32, Lyndon Brown wrote:
> Okay then. v2 attached which just switches to a signed param as you
> suggested.
> 
> Although it may be arguable in some sense to keep it unsigned and have
> the callers avoid calling the method if/when they have a negative
> index, all things considered, I concede that there are perfect good, if
> not greater arguments for just switching to a signed param as you
> suggest in this case. :)
> 
> Regards,
> Lyndon
> 
> On Mon, 2020-09-21 at 09:45 +0200, Pierre Lamot wrote:
>> Hi,
>> 
>> this class extend QAbstractListModel, which uses uses signed indexes
>> (in
>> QModelIndex).
>> negatives values represent invalid indexes.
>> 
>> regards,
>> 
>> On 2020-09-18 23:07, Lyndon Brown wrote:
>> > I have to disagree on this. The function parameter is an index, and
>> > it
>> > is good design to forbid signed values unless there's a good
>> > reason,
>> > such as -1 having a special meaning, (if such things cannot be
>> > easily
>> > communicated by other means). I see no legit reason for opening up
>> > passing in signed values here, which would then require
>> > implementing a
>> > check to reject negative values, and raise issues of UB wrt. signed
>> > value calculations and overflow.
>> >
>> > With respect, I don't think that the fact of callers using signed
>> > variables to handle the index passed to this function is a valid
>> > argument for changing the function to take a signed value.
>> >
>> > If indeed the calling code is handling signed indexes as you say (I
>> > haven't looked myself), then a question needs to be raised as to
>> > why;
>> > an assessment done as to whether that can be changed; and
>> > assessment
>> > done to ensure that a negative value is not incorrectly being
>> > passed to
>> > the function, which would cause a likely out of bounds access.
>> >
>> > Regards,
>> > Lyndon
>> >
>> > On Fri, 2020-09-18 at 09:37 +0200, Pierre Lamot wrote:
>> > > Hi, I think this should probably be the other way around
>> > >
>> > > The method is either called with as argument `int
>> > > QModelIndex::row()`
>> > > or
>> > > an int index.
>> > > you should rather change the argument from unsigned to signed.
>> > >
>> > > On 2020-09-17 23:06, Lyndon Brown wrote:
>> > > > currently a ton of warnings are printed in the build output due
>> > > > to
>> > > > a
>> > > > couple of checks that treat an unsigned var as though signed.
>> > > > the
>> > > > attached patch fixes the problem.
>> > > >
>> > > > _______________________________________________
>> > > > vlc-devel mailing list
>> > > > To unsubscribe or modify your subscription options:
>> > > > https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0001-qt-fix-incorrect-param-type.patch
Type: text/x-diff
Size: 6362 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200922/e1a95587/attachment-0001.patch>