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

[Lyndon Brown]

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: qt_unsigned_v2.patch
Type: text/x-patch
Size: 1139 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200921/909781c6/attachment-0001.bin>