ZheNing Hu [off-list ref] 于2021年5月28日周五 下午11:04写道：

quoted

Can the change in this commit violate the invariant that
if_then_else->str cannot be NULL, which seems to have been the case
forever as we see an unchecked strcmp() done in the original?

If so, perhaps you can check the condition upfront, where you
compute str_len above, e.g.

        if (!if_then_else->str) {
                if (if_then_else->cmp_status == COMPARE_EQUAL ||
                    if_then_else->cmp_status == COMPARE_UNEQUAL)
                        BUG(...);
        } else
                str_len = strlen(...);

If not, then I do not see the point of adding this (and later) check
with BUG to this code.

Or is the invariant that .str must not be NULL could have been
violated without this patch (i.e. the original was buggy in running
strcmp() on .str without checking)?  If so, please make it a separate
preliminary change to add such an assert.

The BUG() here actually acts as an "assert()". ".str must not be NULL" is
right, it point to "xxx" in "%(if:equals=xxx)", so it seems that these BUG()
are somewhat redundant, I will remove them.

Correct the error: If the atom is "%(if)" instread of
"%(if:equals=xxx)", .str will be NULL.
Without assert() or BUG() is ok, but clang-tidy will give a warning:
"Null pointer
passed to 1st parameter expecting 'nonnull'"

--
ZheNing Hu