Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self

From: Elijah Newren <newren@gmail.com>
Date: Mar 14, 2025, 00:24 UTC
Message-ID: <CABPp-BG-7RYskvG6K+gYYNQxqRkVSQYLPvcFaFaT3u-w3bBrYg@mail.gmail.com>
In-Reply-To: <xmqqcyekok4g.fsf@gitster.g>

On Thu, Mar 13, 2025 at 11:34 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > "if (condition) BUG()" is invalid; it needs more arguments.  "if
> > (condition) BUG(something)" requires a separate "something", which
> > requires awkward additional wording and/or is needlessly duplicative.
>
> Ah, obviously we differ on that point.
>
> I consider it an advantage that <something> can be more descriptive
> in a developer friendly way than <condition> expression alone.

I think there's been a communication disconnect. I too like "if (condition) BUG (something)" and often use it, as I pointed out earlier in this thread. The fact that it _can_ be more descriptive is an advantage in some situations (and you provided a great example.)

The problem is that some situations != all situations, and I find it worse when trying to force it for all the other cases. If we still disagree on that point, that's fine, but I wanted to make it clear that I am a fan of and use "if (condition) BUG (something)" where it fits.

[...]

Show 7 quoted lines

> > If you don't want to see assert in the codebase because of NDEBUG,
> > then obviously we'd leave NDEBUG out of BUG_ON().
>
> Absolutely, because the largest problem with assert() is that the
> condition can be compiled away while the compiler does not help
> ensure that the condition part is free of side effects.  If we drop
> NDEBUG, that problem goes away.

Or we can just add a static analysis job that will error whenever the compiler/linker can't prove that assertions have no side effects, and suggest folks use an alternative macro for those instances. I just submitted a series to do that over here: https://lore.kernel.org/git/pull.1881.git.1741911652.gitgitgadget@gmail.com/

That should be far more thorough than any CodingGuideline at making sure we don't have asserts with side effects, be far less effort than attempting to change our several hundred existing assert() invocations in the code base, and should be enough to consider the problem solved and let us move on to something more interesting.