Hello
just one small notices
I dislike a name "root_bool_expr", because, there is not a expression,
but expression type. Can you use "root_bool_expr_type" instead? It is
little bit longer, but more correct. Same not best name is
"root_char", maybe "root_bool_op_name"
or root_expr_type and root_op_name ???
Have no other comments
Regards
Pavel
2013/6/30 Gurjeet Singh <gurjeet@singh.im>:
> On Tue, Jun 18, 2013 at 3:01 PM, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>>
>>
>> related to
>>
>> https://commitfest.postgresql.org/action/patch_view?id=1130
>>
>> http://www.postgresql.org/message-id/CABwTF4V9rsjiBWE+87pK83Mmm7ACdrG7sZ08RQ-4qYMe8jvhbw@mail.gmail.com
>>
>>
>> * motivation: remove recursive procession of AND/OR list (hangs with
>> 10062 and more subexpressions)
>>
>> * patch is short, clean and respect postgresql source code requirements
>> * patch was applied cleanly without warnings
>> * all regression tests was passed
>> * I successfully evaluated expression with 100000 subexpressions
>> * there is no significant slowdown
>>
>> possible improvements
>>
>> a = (A_Expr*) list_nth(pending, 0);
>>
>> a = (A_Expr*) linitial(pending);
>
>
> I made that change, hesitantly. The comments above definition of linitial()
> macro describe the confusion that API causes. I wanted to avoid that
> confusion for new code, so I used the newer API which makes the intention
> quite clear. But looking at that code closely, list_nth() causes at least 2
> function calls, and that's pretty heavy compared to the linitiali() macro.
>
>>
>>
>> not well comment
>>
>> should be -- "If the right branch is also an SAME condition, append it to
>> the"
>
>
> I moved that comment above the outer bock, so that the intention of the
> whole do-while code block is described in one place.
>
>> I don't see any other issues, so after fixing comments this patch is
>> ready for commit
>
>
> Thanks for the review Pavel.
>
> Attached is the updated patch, v4. It has the above edits, and a few code
> improvements, like not repeating the (root_kind == AEPR_AND ? .. : ..)
> ternary expression.
>
> Best regards,
> --
> Gurjeet Singh
>
> http://gurjeet.singh.im/
>
> EnterpriseDB Inc.