On Wed, Aug 22, 2018 at 08:42:20AM -0400, Paul Smith wrote:
> On Tue, 2018-08-21 at 23:03 -0400, Jeff King wrote:
> > static inline int hashcmp(const unsigned char *sha1, const unsigned
> > char *sha2)
> > {
> > + assert(the_hash_algo->rawsz == 20);
> > return memcmp(sha1, sha2, the_hash_algo->rawsz);
> > }
>
> I'm not familiar with Git code, but for most environments assert() is a
> macro which is compiled out when built for "release mode" (whatever
> that might mean). If that's the case for Git too, then relying on
> assert() to provide a side-effect (even an optimizer hint side-effect)
> won't work and this will actually get slower when built for "release
> mode".
>
> Just a thought...
We don't have such a "release mode" in Git, though of course people may
pass -DNDEBUG to the compiler if they want.
However, to me how we spell the assert is mostly orthogonal to the
discussion. We can do "if (...) BUG(...)" to get a guaranteed-present
conditional. The bigger questions are:
- are we OK with such an assertion; and
- does the assertion still give us the desired behavior when we add in
a branch for rawsz==32?
And I think the answers for those are both "probably not".
-Peff
