On 22.12.2014 18:41, Andres Freund wrote:
> On 2014-12-22 18:17:56 +0100, Tomas Vondra wrote:
>> On 22.12.2014 17:47, Alvaro Herrera wrote:
>>> Tomas Vondra wrote:
>>>> On 22.12.2014 07:36, Tatsuo Ishii wrote:
>>>>> On 22.12.2014 00:28, Tomas Vondra wrote:
>>>
>>>>>> (8) Also, I think it's not necessary to define function prototypes for
>>>>>> executeStatement2 and is_table_exists. It certainly is not
>>>>>> consistent with the other functions defined in pgbench.c (e.g.
>>>>>> there's no prototype for executeStatement). Just delete the two
>>>>>> prototypes and move is_table_exists before executeStatement2.
>>>>>
>>>>> I think not having static function prototypes is not a good
>>>>> custom. See other source code in PostgreSQL.
>>>>
>>>> Yes, but apparently pgbench.c does not do that. It's strange to have
>>>> prototypes for just two of many functions in the file.
>>>
>>> Whenever a function is defined before its first use, a prototype is
>>> not mandatory, so we tend to omit them, but I'm pretty sure there are
>>> cases where we add them anyway. I my opinion, rearranging code so
>>> that called functions appear first just to avoid the prototype is not
>>> a very good way to organize things, though. I haven't looked at this
>>> patch so I don't know whether this is what's being done here.
>>
>> I'm not objecting to prototypes in general, but I believe the
>> principle is to respect how the existing code is written. There are
>> almost no other prototypes in pgbench.c - e.g. there are no
>> prototypes for executeStatement(), init() etc. so adding the
>> prototypes in this patch seems inconsistent. But maybe that's
>> nitpicking.
>
> I don't see a point in avoiding prototypes if the author thinks it
> makes his patch clearer. And it's not like pgbench is the pinnacle
> of beauty that would be greatly disturbed by any changes to its form.
I'm not recommending to avoid them (although I'm not sure they make the
patch/code cleaner in this case). It was just a minor observation that
the patch introduces prototypes into code where currently are none. So
executeStatement2() has a prototype while executeStatement() doesn't.
This certainly is not a problem that would make the patch uncommitable,
and yes, it's up to the author to either add prototypes or not. I'm not
going to fight for removing or keeping them.
regards
Tomas
