Re: pgAgent: C++ Port - Patch Review

Hi,

On Thu, Oct 24, 2013 at 2:59 PM, Linreg <linreg(at)gmx(dot)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
>
>
>