Tarantool development patches archive
 help / color / mirror / Atom feed
From: Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state
Date: Sun, 27 Jun 2021 22:42:17 +0300	[thread overview]
Message-ID: <YNjUmRkgEtL1rdIE@grain> (raw)
In-Reply-To: <4b0f4d8f-4e0c-00d0-38ad-0b6abad3aafe@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

> > 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.

  reply	other threads:[~2021-06-27 19:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-25 10:07 Cyrill Gorcunov via Tarantool-patches
2021-06-25 21:49 ` Vladislav Shpilevoy via Tarantool-patches
2021-06-25 21:49   ` Vladislav Shpilevoy via Tarantool-patches
2021-06-26 13:34   ` Cyrill Gorcunov via Tarantool-patches
2021-06-27 14:12     ` Vladislav Shpilevoy via Tarantool-patches
2021-06-27 19:42       ` Cyrill Gorcunov via Tarantool-patches [this message]
2021-07-02 13:43       ` Cyrill Gorcunov via Tarantool-patches
2021-07-07 21:25         ` Vladislav Shpilevoy via Tarantool-patches
2021-07-07 21:59           ` Cyrill Gorcunov via Tarantool-patches
2021-07-07 22:05             ` Cyrill Gorcunov via Tarantool-patches
2021-07-08  9:28               ` [Tarantool-patches] [PATCH v3] raft: change request state to uint64_t Cyrill Gorcunov via Tarantool-patches
2021-07-08 21:17                 ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YNjUmRkgEtL1rdIE@grain \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox