On Thu, Jun 22, 2017 at 05:33:44PM +0200, Emmanuel Deloget wrote: > Hi Antonio, Steffan, > > On Thu, Jun 22, 2017 at 3:31 PM, Antonio Quartulli <a...@unstable.cc> wrote: > > > Thanks for sending v2 Steffan, > > > > On Wed, Jun 21, 2017 at 11:10:43PM +0200, Steffan Karger wrote: > > > From: Steffan Karger <steffan.kar...@fox-it.com> > > > > > > misc.c is a mess of incoherent functions, and is therefore included by > > > virtually all our source files. That makes testing harder than it should > > > be. As a first step of cleaning up misc.c, move adjust_power_of_2() to > > > integer.h, which is a more suitable place for a function like this. > > > > > > This allows us to remove the duplicate implementation from test_argv.c. > > > > > > Signed-off-by: Steffan Karger <steffan.kar...@fox-it.com> > > > > > > This change is quite simple and passes basic tests. > > Thanks also for adding explicit #include statements. > > > > Acked-by: Antonio Quartulli <anto...@openvpn.net> > > > > > Since the code is moved, would it be interesting to use a less greedy > algorithm to do the same thing? I know the function is not used very often, > so it does not look like it's necessary (it's a suggestion, not a > commentary on your patch Steffan :))
My personal opinion: *never* mix code refactoring with functional changes (like
what you are suggesting).
Therefore we'd still need another patch.
>
> For example, this code:
>
> size_t
> adjust_power_of_2(size_t v)
> {
> if (!v)
> return 1;
> v--;
> v |= v >> 1;
> v |= v >> 2;
> v |= v >> 4;
> v |= v >> 8;
> v |= v >> 16;
> if (sizeof(size_t) == 8)
> v |= v >> 32;
> v++;
>
> return v;
> }
>
> does the exact same work on a 64 size_t but will be far faster than the
> loop-based one on many situation (the sizeof() test should be optimized
> away if it's never true, so the code will work even with a 32 bit size_t).
>
> Why it works:
> * if (v - 1) has 0 bits set, then v = 1 and the function will return 1 ((0
> | 0 | ... | 0) + 1)
> * if (v - 1) has at least one bit set (let's say it's bit n), then the
> first iteration will set bit (n-1). The next iteration will set bits (n-2)
> and (n-3). We goes on for a few steps. Before the last iteration, v has
> bits (n...(n-32)) set (with sizeof(size_t) == 8), so the very last
> operation fills the remaining bits and v has bits (n...0) set. Adding one
> to this value will yield the desired result.
>
> Again, the function is not used extensively, so you can ignore this
> suggestion as it will not speed up openvpn at all :)
yeah...not sure we need to make some code a bit "obscure" if there is really no
need.
Still, I'd suggest to prepare a second patch and propose it over the list.
Cheers,
--
Antonio Quartulli
signature.asc
Description: Digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
