Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Sun, Jun 27, 2021 at 04:12:01PM +0200, Vladislav Shpilevoy wrote:
...
> > 
> > thus if compiler use signed int type as internal representation then
> > (int)req->state >= (int)raft_state_MAX won't take the branch because
> > it is a negative value.
> 
> The very fact you needed to put so much effort into the explanation
> of a simple negative number check means it is a huge overkill. Please,
> compare with < 0 explicitly. If the compiler will store the enum as uint,
> it will drop your condition as unreachable anyway.

I put such detailed description because the proper easier fix is to convert
the value to unsigned int _explicitly_ (due to implicit type conversion
compiler does and it is implementation defined, you simply don't know
what exactly compiler does under the hood) and compare it with max,
but I've obviously failed to convince you. Never mind.

> >>
> >> Please, perform a normal < 0 comparison. No need for unsigned cast
> >> cast.
> > 
> > We need an explicit cast here because as I said the internal
> > implementation is compiler defined.
> 
> It has nothing to do with the compiler-specific implementation.

The whole issue is about compiler internals.

> It is simply over-engineering. You cast value to unsigned to
> check if it is negative. WTF? Lets do a normal comparison.

I don't get your "wtf" here, what is wrong with using two complement
properties?

> > On the other hands I played with a number of gcc and llvm compilers
> > and for this code they all does correct comparision (as unsigned
> > integers) so maybe we could just leave it as is, simply close
> > the bug as won't implement and spend time on more important things?
> > I don't mind actually, so up to you just say me what you think.
> 
> I don't mind fixing the issue. But now your patch does not fix it,
> unfortunately. If you want to fix it - go on. But it must be truly
> fixed then.

I rather switch to another bug. The patch fixes exactly what it should,
nothing more, nothing less. But this is your code in first place thus
if you don't like the way I patched it, I suppose it is quite fine
and there is nothing to argue about because the code author usually
selects what better fits his architecture. And taking into account
that modern compilers doesn't cause problems here we can safely
live with it.