Re: [PATCH v3 00/14] Trace2 tracing facility

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 31, 2019, 18:51 UTC
Message-ID: <xmqqbm3w3bjz.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <9b603fd7-d8f0-1ce7-7c7c-8b17484d7d0a@jeffhostetler.com>

Jeff Hostetler <git@jeffhostetler.com> writes:

Show 16 quoted lines

>>>       -+	for_each_builtin(j, tgt_j) {
>>>       ++	for_each_builtin(j, tgt_j)
>>>       ++	{
>>>        +		tgt_j->pfn_term();
>>>        +	}
>>
>> Our CodingGuidelines prefer the opening brace on the same line after
>> the if/for/while/struct/etc. statement, and even omitting the braces
>> if the if arm or loop body consists of a single statement.  So
>> unfortunately a considerable part of this range diff goes in the wrong
>> direction.
>
> I know they do and I had them on the same line originally.
>
> Clang-format was complaining about every use of the for_each_builtin
> macro, so I changed them to be on the next line to quiet it.

Well, clang-format is wrong then ;-)

> I hesitate to remove braces around a statement adjacent to a
> for_each macro trick for the usual safety reasons.

Sorry, but what's "usual safety reasons"? Isn't a macro that requires {} in order to work correctly simply broken?

I see (from a previous iteration---sorry, but I haven't caught up)

#define for_each_builtin(j, tgt_j)                      \
        for (j = 0, tgt_j = tr2_tgt_builtins[j];        \
             tgt_j;                                     \
             j++, tgt_j = tr2_tgt_builtins[j])

and I do not think

	for (j = 0, tgt_j = ...; tgt_j; j++, tgt_j = ...)
		statement;

is unsafe (iow, your macro is not broken).

Puzzled.