[Libreoffice] signed/unsigned comparison (was: [PATCH] Replace SvULongs with vector and code clean up part 1)

[Lubos Lunak]

On Wednesday 10 of August 2011, Stephan Bergmann wrote:
> On Aug 10, 2011, at 12:31 PM, Lubos Lunak wrote:
> > On Tuesday 09 of August 2011, Stephan Bergmann wrote:
> >> Technically, lostd::list is no longer a container, as it violates the
> >> requirement that the return type of size() is size_type.  (And if you
> >> redefine size_type as int, as you should do anyway in the above sketch,
> >> it violates the requirement that size_type is an unsigned integral
> >> type.)
> >
> > Do you realize that as long as the list does not contain 2E9 items, which
> > it does not, this does not matter at all?
>
> That's not my point.  My point is that such an IMO hacky solution that
> tries to outsmart the language is probably not worth it.  (Imagine a
> compiler that emits a warning if a class that does not meet the container
> requirements is used with one of the standard algorithms…)

 This scores really very very low on my scale of hackiness. Can you come up 
with an example that would be broken by this that would not be more hacky on 
its own?

 And actually, there is even nothing wrong with it technically. If you use the 
class with anything that requires size_t to be unsigned, you use the class as 
std::vector, and there size_t is unsigned. It's only the wrapper that is 
signed.

 BTW, Qt has been doing this since at least Qt4.0 (see 
http://doc.qt.nokia.com/4.7/qvector.html#size_type-typedef) and apparently 
has been getting away with it nicely. There used to be exactly the same 
annoyances with older Qt and probably most people by now even don't even know 
there could be a problem with anything.

> >> Really, I would not try to outsmart the specification---even if the
> >> specification is far from beautiful.
> >
> > Right, doing things properly, for whatever definition of 'properly', is
> > more important than anything else. That's how OOo has always been
> > developed and that's why we now have a clean, easy to understand and
> > elegant codebase. Oh, wait.
>
> I hope my post did not come across as demanding a sarcastic reply.

 Sorry. Seeing with some parts of OOo what the result of sticking to doing 
things "properly" not matter what can be, I found it hard to resist.

> >> From my experience, I consider the problem of "randomly added casts" as
> >> not that severe, anyway.  The best fix for the code in question would
> >> probably be if "indexing types" like the type of nEntry were std::size_t
> >> to begin with.
> >
> > You can never do without signed types, so as long as there's a single
> > unsigned type, there always will be mixing.
>
> Yes, we appear to agree that there is some mixing.  I just argued that I do
> not consider the (infrequent) mixing a problem worth a more hacky and less
> straightforward solution than inserting an explicit cast.  (And that if
> there is high-frequency mixing then probably something else is bad about
> the code, and fixing that root cause will also remove the high-frequency
> mixing.)

 The advantage of fixing the root cause (getting rid of unsigned) is that it 
confines the possibly problematic code to one place, which is rather explicit 
about it being so, and does not require everybody to be aware of the traps of 
signed/unsigned problems. IMO it's much safer to have one big marked problem 
than a minefield of bandaided tiny problems (have you never run into a bug 
caused by needless casts? I have).

-- 
 Lubos Lunak
 l.lunak at suse.cz