Re: [ANNOUNCE] Git v2.19.0-rc0
- From
Jeff King <peff@peff.net>
- Date
- Aug 22, 2018, 15:23 UTC
- Message-ID
- <20180822152306.GC32630@sigill.intra.peff.net>
- In-Reply-To
- <814549a01074e89a4b26cb0cf13e4dddeb3a040a.camel@mad-scientist.net>
On Wed, Aug 22, 2018 at 08:42:20AM -0400, Paul Smith wrote:
Show 16 quoted lines
> 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