Re: [PATCH] mailinfo.c: move side-effects outside of assert
Jeff King writes:
> Well, no, I mostly just said that I do not think there is any point in
> defining NDEBUG in the first place, as there is little or no benefit to
> removing those asserts from the built product.
> ...
> Sure, if you want to mass-convert them, doing so with a macro similar to
> assert is the simplest way. I don't think we are in a huge hurry to do
> that conversion though. I'm not complaining that NDEBUG works as
> advertised by disabling asserts. I'm just claiming that it's largely
> pointless in our code base, and I'd consider die("BUG") to be our
> "usual" style.
I agree with all of the above. Given the way how our own code uses
assert(), there is little point removing them and turning them over
time into "if (...) die(BUG)" would probably be better.
Borrowed code like nedmalloc may be a different story, but as you
said in a separate message in this thread, I think we are better off
leaving that to those who care about that piece of code.
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Wed, Dec 21, 2016 at 07:53:15PM -0800, Kyle J. McKay wrote: > It seems to me what you are saying is that Git's "assert" calls are > DIAGNOSTIC and therefore belong in a release build -- well, except for the > nedmalloc "assert" calls which do not. Yes, I think that is a good way of thinking about it (modulo that I really can't say one way or the other about nedmalloc's uses). There _are_ some DEBUG-type things in Git that are protected by #ifdefs that default to "off" (grep for DIFF_DEBUG, for instance). I'm actually of the opinion that debugging code like that should be in all builds and triggerable at run-time, provided it carries no significant performance penalty when the run-time switch is not enabled. But I do agree that's a totally separate question than from your DEBUG/DIAGNOSTIC distinction. -Peff
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Dec 21, 2016, at 18:21, Kyle J. McKay wrote:
On Dec 21, 2016, at 07:55, Jeff King wrote:
On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:
I wasn't aware anybody actually built with NDEBUG at all. You'd
have to
explicitly ask for it via CFLAGS, so I assume most people don't.
Not a good assumption. You know what happens when you assume[1],
right? ;)
Kind of. If it's a configuration that nobody[1] in the Git
development
community intended to support or test, then isn't the person
triggering
it the one making assumptions?
No, I don't think so. NDEBUG is very clearly specified in POSIX [1].
If NDEBUG is defined then "assert(...)" disappears (and in a nice
way so as not to precipitate "unused variable" warnings). "N" being
"No" or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning
Not DEBUG. So the code that goes away when NDEBUG is defined is
clearly debug code.
I think there is a useful distinction here that I make that's worth
sharing. Perhaps it's splitting hairs, but I categorize this "extra"
code that we've been discussing ("assert(...)" or "if (!...) die(...)"
or "verify(...)" into two groups:
1) DEBUG code
This is code that developers use when creating new features. Or
helpful code that's needed when stepping through a program with the
debugger to debug a problem. Or even code that's only used by some
kind of external "test". It may be expensive, it may do things that
should never be done in a build for wider consumption (such as write
information to special log files, write special syslog messages
etc.). Often this code is used in combination with a "-g" debug
symbols build and possibly even a "-O0" or "-O1" option.
Code like this has no place in a release executable meant for general
use by an end user.
2) DIAGNOSTIC code
This is near zero overhead code that is intended to be left in a
release build meant for general use and normally sits there not doing
anything and NOT leaching any performance out of the build either.
Its sole purpose in life is to provide a trail of "bread crumbs" if
the executable goes ***BOOM***. These "bread crumbs" should be just
enough when combined with feedback from the unfortunate user who
experienced the meltdown to re-create the issue in a real DEBUG build
and find and fix the problem.
It seems to me what you are saying is that Git's "assert" calls are
DIAGNOSTIC and therefore belong in a release build -- well, except for
the nedmalloc "assert" calls which do not.
What I'm saying is if they are diagnostic and not debug (and I'm not
arguing one way or the other, but you've already indicated they are
near zero overhead which suggests they are indeed diagnostic in
nature), then they do not belong inside an "assert" which can be
disabled with "NDEBUG". I'm arguing that "assert" is not intended for
diagnostic code, but only debug code as used by nedmalloc. Having Git
treat "NDEBUG" one way -- "no, no, do NOT define NDEBUG because that
disables Git diagnostics and I promise you there's no performance
penalty" -- versus nedmalloc -- "yes, yes please DO define NDEBUG
unless you really need our slow debugging code to be present for
debugging purposes" -- just creates needless unnecessary confusion.
--Kyle
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Wed, Dec 21, 2016 at 06:21:37PM -0800, Kyle J. McKay wrote:
> > Kind of. If it's a configuration that nobody[1] in the Git development
> > community intended to support or test, then isn't the person triggering
> > it the one making assumptions?
>
> No, I don't think so. NDEBUG is very clearly specified in POSIX [1].
I know what NDEBUG is, and I know it is widely used in other projects.
My claim is only that I do not think that the use of NDEBUG was
generally considered when people wrote assert() in git. That is
certainly true of me. I don't know about other developers.
> > I think here you are getting into superstition. Is there any single
> > assert() in Git that will actually have an impact on performance?
>
> You have suggested there is and that Git is enabling NDEBUG for exactly that
> reason -- to increase performance:
Sorry if I wasn't clear, but I meant in Git itself, not in the nedmalloc
compat code. I think it's worth considering them separately because the
latter is not part of most Git builds in the first place, and certainly
it is written in a different style, and may have different assumptions
about how people might build it.
I do not know the nedmalloc code well at all. I gave a brief read of the
results of "git grep 'assert('" and it looked like some of its
assertions are more involved than simple comparisons. So I guessed that
perhaps the reason that NDEBUG was added there when the code was
imported is that there is a performance difference there. But that's
just a guess. It may or may not be borne out by measurements.
I'd be surprised if any of the assertions in the rest of git have a
noticeable impact (and I could not detect one by running t/perf).
> > What would be the advantage of that over `if(...) die("BUG: ...")`? It
> > does not require you to write a reason in the die(), but I am not sure
> > that is a good thing.
>
> You have stated that you believe the current "assert" calls in Git
> (excluding nedmalloc) should not magically disappear when NDEBUG is defined.
Well, no, I mostly just said that I do not think there is any point in
defining NDEBUG in the first place, as there is little or no benefit to
removing those asserts from the built product.
> So precluding a more labor intensive approach where all currently existing
> "assert(...)" calls are replaced with an "if (!...) die(...)" combination,
> providing a "verify" macro is a quick way to make that happen. Consider
> this, was the value that Jonathan provided for the "die" string immediately
> obvious to you? It sure wasn't to me. That means that whoever does the
> "assert(...)" -> "if(!...)die" swap out may need to be intimately familiar
> with that particular piece of code or the result will be no better than
> using a "verify" macro.
Sure, if you want to mass-convert them, doing so with a macro similar to
assert is the simplest way. I don't think we are in a huge hurry to do
that conversion though. I'm not complaining that NDEBUG works as
advertised by disabling asserts. I'm just claiming that it's largely
pointless in our code base, and I'd consider die("BUG") to be our
"usual" style. If any change is to be made, it would be to suggest
people prefer die("BUG") as a style guideline for future patches. But I
haven't seen anybody agree (or disagree) with the notion, so I'd
hesitate to impose my style suggestion without more discussion in that
area.
If we _did_ accept that as a style guideline, then we could move over
existing assert() calls over time (either as janitorial projects, or as
people touch the related code). But there's not a pressing need to do it
quickly.
-Peff
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Dec 21, 2016, at 07:55, Jeff King wrote:
On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:
I wasn't aware anybody actually built with NDEBUG at all. You'd
have to
explicitly ask for it via CFLAGS, so I assume most people don't.
Not a good assumption. You know what happens when you assume[1],
right? ;)
Kind of. If it's a configuration that nobody[1] in the Git development
community intended to support or test, then isn't the person
triggering
it the one making assumptions?
No, I don't think so. NDEBUG is very clearly specified in POSIX [1].
If NDEBUG is defined then "assert(...)" disappears (and in a nice way
so as not to precipitate "unused variable" warnings). "N" being "No"
or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning Not
DEBUG. So the code that goes away when NDEBUG is defined is clearly
debug code.
Considering the wide deployment and use of Git at this point I think
rather the opposite to be true that "Git does Not require DEBUGging
code to be enabled for everyday use." The alternative that it does
suggests it's not ready for prime time and quite clearly that's not
the case.
I've been defining NDEBUG whenever I make a release build for quite
some
time (not just for Git) in order to squeeze every last possible
drop of
performance out of it.
I think here you are getting into superstition. Is there any single
assert() in Git that will actually have an impact on performance?
You have suggested there is and that Git is enabling NDEBUG for
exactly that reason -- to increase performance:
On Dec 20, 2016, at 08:45, Jeff King wrote:
I do notice that we set NDEBUG for nedmalloc, though if I am
reading the
Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive
Perhaps Git should provide a "verify" macro. Works like "assert"
except
that it doesn't go away when NDEBUG is defined. Being Git-provided
it could
also use Git's die function. Then Git could do a global replace of
assert
with verify and institute a no-assert policy.
What would be the advantage of that over `if(...) die("BUG: ...")`? It
does not require you to write a reason in the die(), but I am not sure
that is a good thing.
You have stated that you believe the current "assert" calls in Git
(excluding nedmalloc) should not magically disappear when NDEBUG is
defined. So precluding a more labor intensive approach where all
currently existing "assert(...)" calls are replaced with an "if (!...)
die(...)" combination, providing a "verify" macro is a quick way to
make that happen. Consider this, was the value that Jonathan provided
for the "die" string immediately obvious to you? It sure wasn't to
me. That means that whoever does the "assert(...)" -> "if(!...)die"
swap out may need to be intimately familiar with that particular piece
of code or the result will be no better than using a "verify" macro.
I'm just trying to find a quick and easy way to accommodate your
wishes without redefining the semantics of NDEBUG. ;)
--Kyle
[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/assert.h.html
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:
> > I wasn't aware anybody actually built with NDEBUG at all. You'd have to
> > explicitly ask for it via CFLAGS, so I assume most people don't.
>
> Not a good assumption. You know what happens when you assume[1], right? ;)
Kind of. If it's a configuration that nobody[1] in the Git development
community intended to support or test, then isn't the person triggering
it the one making assumptions?
At any rate, I agree that setting NDEBUG should not create a broken
program, and some solution like your patch is a good idea here. I was
mainly speaking to the "do not bother" comment. It is not that I do not
bother to build with NDEBUG, it is that I think it is actively a bad
idea.
[1] Maybe I am alone in my surprise, and everybody working on Git is
using assert() with the intention that it can be disabled. But if
that were the case, I'd expect more push-back against "die(BUG)"
which does not have this feature. I don't recall a single discussion
to that effect, and searching for NDEBUG in the list archives turns
up hardly any mentions.
> I've been defining NDEBUG whenever I make a release build for quite some
> time (not just for Git) in order to squeeze every last possible drop of
> performance out of it.
I think here you are getting into superstition. Is there any single
assert() in Git that will actually have an impact on performance?
I'd be more impressed if you could show some operation that is faster
when built with NDEBUG than without. Running all of t/perf does not seem
to show any difference, and looking at the asserts themselves, they're
almost all single-instruction compares in code that isn't performance
critical anyway.
> > So from my perspective it is not so much "do not bother with release
> > builds" as "are release builds even a thing for git?"
>
> They should be if you're deploying Git in a performance critical
> environment.
I hope my history of patches shows that I do care about deploying Git in
a performance critical environment. But I only care about performance
tradeoffs that have a _measurable_ gain.
> Perhaps Git should provide a "verify" macro. Works like "assert" except
> that it doesn't go away when NDEBUG is defined. Being Git-provided it could
> also use Git's die function. Then Git could do a global replace of assert
> with verify and institute a no-assert policy.
What would be the advantage of that over `if(...) die("BUG: ...")`? It
does not require you to write a reason in the die(), but I am not sure
that is a good thing.
> > I do notice that we set NDEBUG for nedmalloc, though if I am reading the
> > Makefile right, it is just for compiling those files. It looks like
> > there are a ton of asserts there that _are_ potentially expensive, so
> > that makes sense.
>
> So there's no way to get a non-release build of nedmalloc inside Git then
> without hacking the Makefile? What if you need those assertions enabled?
> Maybe NDEBUG shouldn't be defined by default for any files.
AFAICT, yes. I'd leave it to people who actually build with nedmalloc to
decide whether it is worth caring about, and whether the asserts there
have a noticeable performance impact.
-Peff
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Dec 20, 2016, at 08:45, Jeff King wrote:
On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:
On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
ACK. I noticed this problem (and fixed it independently as a part
of a
huge patch series I did not get around to submit yet) while
trying to
get Git to build correctly with Visual C.
Does this mean that Dscho and I are the only ones who add -DNDEBUG
for
release builds? Or are we just the only ones who actually run the
test
suite on such builds?
It seems you and I are for the moment the only ones bothering with
running
the test suite on release builds.
I wasn't aware anybody actually built with NDEBUG at all. You'd have
to
explicitly ask for it via CFLAGS, so I assume most people don't.
Not a good assumption. You know what happens when you assume[1],
right? ;)
I've been defining NDEBUG whenever I make a release build for quite
some time (not just for Git) in order to squeeze every last possible
drop of performance out of it.
Certainly I never have when deploying to GitHub's cluster (let alone
my
personal use), and I note that the Debian package also does not.
Yeah, I don't do it for my personal use because those are often not
based on a release tag so I want to see any assertion failures that
might happen and they're also not performance critical either.
So from my perspective it is not so much "do not bother with release
builds" as "are release builds even a thing for git?"
They should be if you're deploying Git in a performance critical
environment.
One of the
reasons I suggested switching the assert() to a die("BUG") is that the
latter cannot be disabled. We generally seem to prefer those to
assert()
in our code-base (though there is certainly a mix). If the assertions
are not expensive to compute, I think it is better to keep them in for
all builds. I'd much rather get a report from a user that says "I hit
this BUG" than "git segfaulted and I have no idea where" (of course I
prefer a backtrace even more, but that's not always an option).
Perhaps Git should provide a "verify" macro. Works like "assert"
except that it doesn't go away when NDEBUG is defined. Being Git-
provided it could also use Git's die function. Then Git could do a
global replace of assert with verify and institute a no-assert policy.
I do notice that we set NDEBUG for nedmalloc, though if I am reading
the
Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive, so
that makes sense.
So there's no way to get a non-release build of nedmalloc inside Git
then without hacking the Makefile? What if you need those assertions
enabled? Maybe NDEBUG shouldn't be defined by default for any files.
--Kyle
[1] https://www.youtube.com/watch?v=KEP1acj29-Y
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:
> > On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
> >
> > >ACK. I noticed this problem (and fixed it independently as a part of a
> > >huge patch series I did not get around to submit yet) while trying to
> > >get Git to build correctly with Visual C.
> >
> > Does this mean that Dscho and I are the only ones who add -DNDEBUG for
> > release builds? Or are we just the only ones who actually run the test
> > suite on such builds?
>
> It seems you and I are for the moment the only ones bothering with running
> the test suite on release builds.
I wasn't aware anybody actually built with NDEBUG at all. You'd have to
explicitly ask for it via CFLAGS, so I assume most people don't.
Certainly I never have when deploying to GitHub's cluster (let alone my
personal use), and I note that the Debian package also does not.
So from my perspective it is not so much "do not bother with release
builds" as "are release builds even a thing for git?". One of the
reasons I suggested switching the assert() to a die("BUG") is that the
latter cannot be disabled. We generally seem to prefer those to assert()
in our code-base (though there is certainly a mix). If the assertions
are not expensive to compute, I think it is better to keep them in for
all builds. I'd much rather get a report from a user that says "I hit
this BUG" than "git segfaulted and I have no idea where" (of course I
prefer a backtrace even more, but that's not always an option).
I do notice that we set NDEBUG for nedmalloc, though if I am reading the
Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive, so
that makes sense.
-Peff
Re: [PATCH] mailinfo.c: move side-effects outside of assert
Hi Kyle, On Mon, 19 Dec 2016, Kyle J. McKay wrote: > On Dec 19, 2016, at 09:45, Johannes Schindelin wrote: > > >ACK. I noticed this problem (and fixed it independently as a part of a > >huge patch series I did not get around to submit yet) while trying to > >get Git to build correctly with Visual C. > > Does this mean that Dscho and I are the only ones who add -DNDEBUG for > release builds? Or are we just the only ones who actually run the test > suite on such builds? It seems you and I are for the moment the only ones bothering with running the test suite on release builds. Ciao, Johannes
Re: [PATCH] mailinfo.c: move side-effects outside of assert
Jonathan Tan writes:
>>> This is obviously an improvement, but it makes me wonder if we should be
>>> doing:
>>>
>>> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
>>> die("BUG: some explanation of why this can never happen");
>>>
>>> which perhaps documents the intended assumptions more clearly. A comment
>>> regarding the side effects might also be helpful.
>>
>> I wondered exactly the same thing myself. I was hoping Jonathan would
>> pipe in here with some analysis about whether this is:
>>
>> a) a super paranoid, just-in-case, can't really ever fail because by
>> the time we get to this code we've already effectively validated
>> everything that could cause check_header to return false in this case
>> ...
> The answer is "a". The only time that mi->inbody_header_accum is
> appended to is in check_inbody_header, and appending onto a blank
> mi->inbody_header_accum always happens when is_inbody_header is true
> (which guarantees a prefix that causes check_header to always return
> true).
>
> Peff's suggestion sounds reasonable to me, maybe with an error message
> like "BUG: inbody_header_accum, if not empty, must always contain a
> valid in-body header".
OK. So we do not expect it to fail, but we still do want the side
effect of that function (i.e. accmulation into the field).
Somebody care to send a final "agreed-upon" version?
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On 12/19/2016 12:38 PM, Kyle J. McKay wrote:
On Dec 19, 2016, at 12:03, Jeff King wrote:
On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:
assert(call_a_function(...))
Thanks for spotting this - I'm not sure how I missed that.
This is obviously an improvement, but it makes me wonder if we should be
doing:
if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
die("BUG: some explanation of why this can never happen");
which perhaps documents the intended assumptions more clearly. A comment
regarding the side effects might also be helpful.
I wondered exactly the same thing myself. I was hoping Jonathan would
pipe in here with some analysis about whether this is:
a) a super paranoid, just-in-case, can't really ever fail because by
the time we get to this code we've already effectively validated
everything that could cause check_header to return false in this case
-or-
b) Yeah, it could fail in the real world and it should "die" (and
probably have a test added that triggers such death)
-or-
c) Actually, if check_header does return false we can keep going
without problem
-or-
d) Actually, if check_header does return false we can keep going by
making a minor change that should be in the patch
I assume that since Jonathan added the code he will just know the answer
as to which one it is and I won't have to rely on the results of my
imaginary analysis. ;)
The answer is "a". The only time that mi->inbody_header_accum is
appended to is in check_inbody_header, and appending onto a blank
mi->inbody_header_accum always happens when is_inbody_header is true
(which guarantees a prefix that causes check_header to always return true).
Peff's suggestion sounds reasonable to me, maybe with an error message
like "BUG: inbody_header_accum, if not empty, must always contain a
valid in-body header".
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Dec 19, 2016, at 12:03, Jeff King wrote:
On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:
assert(call_a_function(...))
The function in question, check_header, has side effects. This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.
Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.
Signed-off-by: Kyle J. McKay
---
Notes:
Please include this PATCH in 2.11.x maint
This is obviously an improvement, but it makes me wonder if we
should be
doing:
if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
die("BUG: some explanation of why this can never happen");
which perhaps documents the intended assumptions more clearly. A
comment
regarding the side effects might also be helpful.
I wondered exactly the same thing myself. I was hoping Jonathan would
pipe in here with some analysis about whether this is:
a) a super paranoid, just-in-case, can't really ever fail because
by the time we get to this code we've already effectively validated
everything that could cause check_header to return false in this case
-or-
b) Yeah, it could fail in the real world and it should "die" (and
probably have a test added that triggers such death)
-or-
c) Actually, if check_header does return false we can keep going
without problem
-or-
d) Actually, if check_header does return false we can keep going by
making a minor change that should be in the patch
I assume that since Jonathan added the code he will just know the
answer as to which one it is and I won't have to rely on the results
of my imaginary analysis. ;)
On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
ACK. I noticed this problem (and fixed it independently as a part of a
huge patch series I did not get around to submit yet) while trying
to get
Git to build correctly with Visual C.
Does this mean that Dscho and I are the only ones who add -DNDEBUG for
release builds? Or are we just the only ones who actually run the
test suite on such builds?
--Kyle
Re: [PATCH] mailinfo.c: move side-effects outside of assert
On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
> assert of the form:
>
> assert(call_a_function(...))
>
> The function in question, check_header, has side effects. This
> means that when NDEBUG is defined during a release build the
> function call is omitted entirely, the side effects do not
> take place and tests (fortunately) start failing.
>
> Move the function call outside of the assert and assert on
> the result of the function call instead so that the code
> still works properly in a release build and passes the tests.
>
> Signed-off-by: Kyle J. McKay
> ---
>
> Notes:
> Please include this PATCH in 2.11.x maint
This is obviously an improvement, but it makes me wonder if we should be
doing:
if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
die("BUG: some explanation of why this can never happen");
which perhaps documents the intended assumptions more clearly. A comment
regarding the side effects might also be helpful.
-Peff
Re: [PATCH] mailinfo.c: move side-effects outside of assert
Hi, On Sat, 17 Dec 2016, Kyle J. McKay wrote: > Since 6b4b013f18 (mailinfo: handle in-body header continuations, > 2016-09-20, v2.11.0) mailinfo.c has contained new code with an > assert of the form: > > assert(call_a_function(...)) > > The function in question, check_header, has side effects. This > means that when NDEBUG is defined during a release build the > function call is omitted entirely, the side effects do not > take place and tests (fortunately) start failing. > > Move the function call outside of the assert and assert on > the result of the function call instead so that the code > still works properly in a release build and passes the tests. > > Signed-off-by: Kyle J. McKay ACK. I noticed this problem (and fixed it independently as a part of a huge patch series I did not get around to submit yet) while trying to get Git to build correctly with Visual C. Ciao, Dscho
[PATCH] mailinfo.c: move side-effects outside of assert
Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:
assert(call_a_function(...))
The function in question, check_header, has side effects. This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.
Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.
Signed-off-by: Kyle J. McKay
---
Notes:
Please include this PATCH in 2.11.x maint
mailinfo.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..47442fb5 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -708,9 +708,12 @@ static int is_scissors_line(const char *line)
static void flush_inbody_header_accum(struct mailinfo *mi)
{
+ int okay;
+
if (!mi->inbody_header_accum.len)
return;
- assert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+ okay = check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0);
+ assert(okay);
strbuf_reset(&mi->inbody_header_accum);
}
---
