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

[Lyndon Brown]

No problem. LGTM in principle.

I realised on receiving your reply that I actually got distracted
yesterday and failed to compile v2 to double check there were no other
warnings coming from that.

Anyway, it might be ideal if things were redesigned to not have to deal
at all with negative indexes, but otherwise, I guess it's a bit of an
arbitrary toss up between rejecting them within the function or
externally casting to unsigned, resulting in very large indexes that
then just ultimately get rejected in the same way within the function
from being inevitably always way out of range. It's probably cleaner to
do as you've done in v3 though. So I'm good with v3.

By all means do add mention of your collaborative effort into the
commit log and adjust it as you see fit. In fact there's more change
and effort put in from your end than mine, so in this instance I'd be
completely cool if you'd like to flip things to yourself having the
authorship of this commit. :)

Regards,
Lyndon

On Tue, 2020-09-22 at 10:52 +0200, Pierre Lamot wrote:
> 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