Re: pgAgent: C++ Port - Patch Review

Hi,

On Thu, Oct 24, 2013 at 2:59 PM, Linreg <linreg@gmx.net> wrote:

Am Donnerstag, 24. Oktober 2013, 11:11:21 schrieb Neel Patel:

> Hi,

>

> Below are the review comments. Compile and tested in Linux.

>

> 1) During compilation we got the following error in "connection.h"

>

> error: pgsql/libpq-fe.h: No such file or directory

>

> To solve the above error we changed "#include <libpg-fe.h>" instead of

> "#include <pgsql/libpq-fe.h>" and it is working fine. Correct me if it is

> some configuration issue.

>

> 2) We are getting the below warning in unix.cpp file which needs to fix.

>

> warning: the use of `tmpnam' is dangerous, better use `mkstemp'.

>

>

> 3) Some of the code is commented and not used so remove the unnecessary

> code.

>

> 4) README file should be modified according to new changes.

>

> 5) In Execute() method in Job.cpp file, we should have to delete "steps"

> from wherever we are returning otherwise it will create the memory leak.

>

>

> Thanks,

> Neel Patel

 

1.)

it is system dependent. On my SUSE system pibpg-header to be installed under the pgsql directory.

Example: /usr/include/pgsql/libpq-fe.h

As we discussed we should remove the pgsql/ prefix.

 

 

2.)

I changed tmpnam to mkdtemp.

I hope thats is okay

OK.

 

 

3.)

Can you tell me the name of the files. I will delete comments.

connection.cpp

job.cpp

misc.cpp

pgAgent.h

Also check other files for commented codes.

 

 

4.)

its also my job?

I think so because you have only implemented this feature so you are the best person to do it. :)

 

 

5.)

I have corrected. In the default branch of the switch statement was a return statement(Job.cpp Line: 325). Before that i have a "delete steps," added

That is fine. But what about return statement in Job.cpp:346 ?

 

 

 

Thomas Steffen