[Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Frediano Ziglio]

> 
> > On 20 Jul 2017, at 11:23, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > On Thu, Jul 20, 2017 at 09:42:26AM +0200, Christophe de Dinechin wrote:
> >>> I moved to "unsigned int" 2 versions ago.
> >> 
> >> It was courteous of you.
> > 
> > Yup, thanks, did not take a look at the newer iterations yet.
> > 
> >> 
> >>> 
> >>> But still think that is a useful discussion. But honestly I think
> >>> in this case the readability is quite an opinion and for me
> >>> unsigned is like long, perfectly readable and I saw lot of code
> >>> using just unsigned.
> >> 
> >> It is useful on two levels:
> >> 
> >> a) Because I personally feel ‘unsigned int’ less readable and less
> >> consistent with the common usage for.’long’ or ‘short’.
> > 
> > Talking about consistency, should why are you using 'int' rather than
> > 'signed' if you prefer using 'unsigned’?
> 
> For the same reason that I prefer ‘unsigned’ over ‘unsigned int’ : because
> it is the shortest way to express the given type. Remember ‘WET’,
> “We Enjoy Typing” ? :-)
> 
> > Another way of seeing
> > consistency is:
> > 
> > unsigned long
> > unsigned short
> > unsigned int
> 
> Unsigned long and unsigned short cannot be written in any other way
> without a typedef. But many developers have typedefs such as
> ulong or ushort to avoid what they see as an inconsistency, having
> types that require two words when the vast majority of types require
> only one (see below one implication for C++).
> 
> > 
> > You've got the unsigned modifier, and then something giving a hint at
> > the storage size.
> > 
> > Similarly, you have:
> > 
> > long
> > short
> > int
> > 
> > and you know these usually are signed.
> 
> No, they are always signed. Only char is possibly signed or unsigned, AFAIR.
> See
> https://stackoverflow.com/questions/2054939/is-char-signed-or-unsigned-by-default.
> 
> > 
> >> 
> >> b) To discuss the matter of debating style preferences in code reviews,
> >> notably for new code and new languages.
> >> 
> >> In that case, we have C++ code, where my experience tells me that
> >> ‘unsigned’ is by far more common (as is camel-case, for example).
> >> 
> >> If we start considering that some C programmer may not know that
> >> ‘unsigned’ means ‘unsigned int’, what happens the first time someone
> >> introduces ‘auto’ or ‘for (a : b) in C++ code, or _Generic in C code?
> > 
> > To reuse your numbers, the usage of unsigned VS unsigned int in arch/ in
> > the kernel was 90%/10% or even less than that. So yes, it's not unlikely
> > that someone will never have came across "unsigned", or forgot what it
> > means, …
> 
> Are you saying that since the usage ratio in the Linux kernel is 90%/10%,
> it is likely that regular C developers don’t know what ‘unsigned’ means?
> I disagree, ‘unsigned’ is not an obscure C feature. A developer who does not
> know what ‘unsigned’ means needs to learn it.
> 

I think the Linux kernel is quite a bad example... and still they use
unsigned alone!
First is C, not C++. Second is really low level and far from pure C,
try to compile with a not-gcc compliant compiler.

> > _Generic has no trivial equivalent for C code contrary to
> > "unsigned". Your C++ examples being C++ and not C, I would care less
> > about C programmers not familiar with C++ ;)
> 
> The code under review is C++, isn’t it? C++ users are even more
> wary of two-word type names than C users. There are syntactic constructs
> in C++ that require a one-word name, notably the new constructor-style
> syntax for casts:
> 
> % cat /tmp/glop.cpp
> int long unsigned typedef ulong;
> 
> unsigned ok2 = (unsigned) 1;
> unsigned ok1 = unsigned(1);
> ulong ok3 = (ulong) 1;
> ulong ok4 = ulong(1);
> unsigned long ok5 = (unsigned long) 1;
> unsigned long bad = unsigned long (1);
> ddd at ptitpuce[master] Chapter9> c++ -c /tmp/glop.cpp
> /tmp/glop.cpp:8:30: error: expected '(' for function-style cast or type
>       construction
> unsigned long bad = unsigned long (1);
>                     ~~~~~~~~ ^
> 1 error generated.
> 
> This is one of the reasons for this preference for ‘unsigned’ of all
> serious C++ developers that I know of (that may be a possible reason
> Frediano unconsciously started with ‘unsigned’ in C++ code):
> 

Unconsciously because I saw lot of books, examples and code
using just unsigned, personally is like 20 years I'm using it.

> 
> I’m not sure the discussion is very productive. I only answered because
> I thought there was a factual error about int being ‘usually’ signed
> and because I thought it was worth explaining the C++ preference for
> single-word type names. That being said, I am not sure I can convince
> you, so we’ll probably agree to disagree on this one ;-)
> 

As far as I can see here there are 2 person that prefer just
unsigned and 1 that don't and the reasons are based on personal
preference and thinking in a language which are different from
the one the code is written.

> 
> Cheers,
> Christophe
> 
> > 
> > Christophe

Beside that I wonder why I had to wait 8 months for these reviews,
not counting the time to decide to rewrite this part of code
(with all the wasted time trying to not do it) and the time we
waited to fix a known bug which is also a regression (image copy
paste are not working) and potentially a security risk (the library
versions used contain security bugs but actually the code is disabled).
Looks like that if a Red Hat client is not pushing for a fix
solutions are not that important.

Frediano