On 09/25/2012 06:46 PM, Tanu Kaskinen wrote:
> On Mon, 2012-09-24 at 17:02 +0200, David Henningsson wrote:
>> On 09/24/2012 03:33 PM, Tanu Kaskinen wrote:
>>> Ok, I don't have a problem with this example. But the code in your patch
>>> has different structure: instead of a simple match_found() call, the
>>> inner code contains a three-line block.
>>
>> Well, I still think
>>
>> for (i)
>> for (j)
>> if (a[i] == b[j]) {
>> three();
>> line();
>> block();
>> }
>>
>> ...looks better than
>>
>> for (i)
>> for (j)
>> if (a[i] == b[j]) {
>> three();
>> line();
>> block();
>> }
>> }
>> }
>>
>> ...but maybe that's just me.
>
> I certainly prefer the latter, though this is not a very important thing
> for me. But I'm now left wondering what you thought we agreed earlier -
> unfortunately I don't have the logs, but I think we agreed to disallow
> some cases of omitting the braces. If you think that the first example
> would still be allowed, I don't know what case would be left to
> disallow.
I don't recall for sure, but I think I was imagining multiline
statements, e g
if (i)
this_is_a_very_long_statement(with_multiple, parameters,
that_maybe_are_functions(themselves));
vs
if (i) {
this_is_a_very_long_statement(with_multiple, parameters,
that_maybe_are_functions(themselves));
}
in which case the latter is okay. Anyway, if it's not important, it's
just another trap to fall in when you try to fix somebody's real problem.
>>> Fair enough. I don't like the check, because it implies that it's
>>> possible that output_mappings is empty, which is not true, but I see how
>>> this can prevent a bug in the future (not a severe bug, because only a
>>> minor optimization would be skipped, but a bug anyway). I think it would
>>> be best to always create the output_mappings idxset so that the code
>>> doesn't have to worry about it being NULL. If you agree, I'll file a
>>> bug, since the issue is separate from this patch and I don't plan to
>>> write the patch right away, nor ask you to do so.
>>
>> I don't know if we have a generic opinion on whether we prefer NULL
>> idxsets or empty idxsets? Or determine on a case-by-case basis?
>
> I don't think there has been any rule so far, or if there has been, it
> has been to favor NULL. I would definitely choose to always initialize
> idxsets and hashmaps if there's no particular reason to distinguish
> between NULL and an empty container. (I have even sent some patches that
> ensure that certain idxsets or hashmaps are always non-NULL, but I don't
> remember if they have been reviewed yet.) It makes the code simpler when
> you don't have to worry about the container being NULL.
Another option could be to make pa_hashmap_* to treat NULL pointers as
empty containers. I don't know if that's better though.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
