On two patches that may attempt to do the same things, what's better?

[illwieckz]

On February 16 of 2014 @gimhael committed the patch 4a63c5a named “Linux64 compilation fixes.” doing this:

-  size_t new_capacity = min_new_capacity;
+  ptr_bits_t new_capacity = min_new_capacity;
   if ((grow_hint) && (!math::is_power_of_2(new_capacity)))
     new_capacity = math::next_pow2(new_capacity);

I submitted this patch as PR BinomialLLC#13 to @BinomialLLC on July 23 of 2017 in hope to get it upstreamed but the patch never got merged neither the submission got an answer for unknown reason. This PR is tracked on our side in #2.

I notice today that @richgel999 merged on October 9 of 2020 the PR BinomialLLC#25 with the patch 3a445a9 by @griffin2000 committed on November 13 of 2018 and named “Avoid ambiguity error on GCC” that seems to touch the same code, but doing different things instead:

       size_t new_capacity = min_new_capacity;
-      if ((grow_hint) && (!math::is_power_of_2(new_capacity)))
-         new_capacity = math::next_pow2(new_capacity);
+	  if ((grow_hint) && (!math::is_power_of_2((uint64)new_capacity)))
+		  new_capacity = math::next_pow2((uint64)new_capacity);

1. Are the two patches meant to fix the same bug?
2. Which implementation is the best one?

[illwieckz]

I notice I reported on July 23 of 2017 at @BinomialLLC the issue BinomialLLC#11 as the one fixed by gimhael's patch submitted as PR BinomialLLC#13, the issue was named “ambiguous calls prevents compilation (gcc, clang)” and reported this kind of compilation error:

crn_vector.cpp:26:53: error: call of overloaded ‘next_pow2(size_t&)’ is ambiguous
          new_capacity = math::next_pow2(new_capacity);
                                                     ^
In file included from crn_core.h:173:0,
                 from crn_vector.cpp:3:
crn_math.h:84:21: note: candidate: crnlib::uint32 crnlib::math::next_pow2(crnlib::uint32)
       inline uint32 next_pow2(uint32 val)
                     ^~~~~~~~~
crn_math.h:95:21: note: candidate: crnlib::uint64 crnlib::math::next_pow2(crnlib::uint64)
       inline uint64 next_pow2(uint64 val)

crn_vector.cpp:25:60: error: call of overloaded ‘is_power_of_2(size_t&)’ is ambiguous
       if ((grow_hint) && (!math::is_power_of_2(new_capacity)))
                                                            ^
In file included from crn_core.h:173:0,
                 from crn_vector.cpp:3:
crn_math.h:59:19: note: candidate: bool crnlib::math::is_power_of_2(crnlib::uint32)
       inline bool is_power_of_2(uint32 x) { return x && ((x & (x - 1U)) == 0U); }
                   ^~~~~~~~~~~~~
crn_math.h:60:19: note: candidate: bool crnlib::math::is_power_of_2(crnlib::uint64)
       inline bool is_power_of_2(uint64 x) { return x && ((x & (x - 1U)) == 0U); }
                   ^~~~~~~~~~~~~

The issue also reported that the problem was faced on both GCC and LLVM's clang.

So it looks like the two patches are targetting the same issue.

[illwieckz]

@slipher or @DolceTriade do you have an opinion on which patch may be better and why?

[slipher]

Yes they are fixing the same issue. gimhael's one seems nicer.

[illwieckz]

Do the other patch would work on non-64bit systems?

[slipher]

Do the other patch would work on non-64bit systems?

Yes

[illwieckz]

OK, if both does the same but the one we already merged looks nicer we can keep it then.

Thanks for your answers.