Re: pgScript patch

Hello Dave,

2008/8/4 Dave Page <dpage(at)pgadmin(dot)org>:
> Cool. It is indeed a huge patch, so I'll have to leave it to your
> mentor to undertake a more extensive code review, but here are a few
> points I noticed whilst testing on Mac:
>
> - Please include "Copyright (C) 2002 - 2008, The pgAdmin Development
> Team" in the copyright notices at the top of each source file. I'm
> happy for you to include your own copyright there also, but it makes
> things much easier from a legal POV if you list us as well.

Will be done in pgScript-1.0-beta-2.

> - The build failed initially with:
>
> ./pgscript/statements/pgsStmtList.cpp: In member function 'virtual
> void pgsStmtList::eval(pgsVarMap&) const':
> ./pgscript/statements/pgsStmtList.cpp:56: error: cannot use typeid
> with -fno-rtti
> ./pgscript/statements/pgsStmtList.cpp:56: error: cannot use typeid
> with -fno-rtti
> ./pgscript/statements/pgsStmtList.cpp:57: error: cannot use typeid
> with -fno-rtti
> ./pgscript/statements/pgsStmtList.cpp:57: error: cannot use typeid
> with -fno-rtti
> ./pgscript/statements/pgsStmtList.cpp: In member function 'virtual
> void pgsStmtList::eval(pgsVarMap&) const':
> ./pgscript/statements/pgsStmtList.cpp:56: error: cannot use typeid
> with -fno-rtti
> ./pgscript/statements/pgsStmtList.cpp:56: error: cannot use typeid
> with -fno-rtti
> ./pgscript/statements/pgsStmtList.cpp:57: error: cannot use typeid
> with -fno-rtti
> ./pgscript/statements/pgsStmtList.cpp:57: error: cannot use typeid
> with -fno-rtti
> lipo: can't figure out the architecture type of:
> /var/folders/uk/ukdzizfJHxe07gKAk8a+NE+++TI/-Tmp-//ccIbnqxz.out
> make[2]: *** [pgsStmtList.o] Error 1
> make[1]: *** [all-recursive] Error 1
> make: *** [all] Error 2

Indeed, I am using virtual methods. Is there any reasons why you
compiled with -fno-rtti ? Do I have to use wxWidgets RTTI capabilities
instead of C++ built-in capabilities?

> After removing -fno-rtti from acinclude.m4:
>
> - I see the following warnings:
>
> ./pgadmin/include/frm/frmQuery.h: In constructor
> 'frmQuery::frmQuery(frmMain*, const wxString&, pgConn*, const
> wxString&, const wxString&)':
> ../pgadmin/include/frm/frmQuery.h:75: warning: 'frmQuery::pgscript'
> will be initialized after
> ../pgadmin/include/frm/frmQuery.h:71: warning: 'wxTimer frmQuery::timer'
> ./frm/frmQuery.cpp:130: warning: when initialized here
> ../pgadmin/include/frm/frmQuery.h: In constructor
> 'frmQuery::frmQuery(frmMain*, const wxString&, pgConn*, const
> wxString&, const wxString&)':
> ../pgadmin/include/frm/frmQuery.h:75: warning: 'frmQuery::pgscript'
> will be initialized after
> ../pgadmin/include/frm/frmQuery.h:71: warning: 'wxTimer frmQuery::timer'
> ./frm/frmQuery.cpp:130: warning: when initialized here

Will fix that in the next patch.

> - The following script crashes (yes, I realise it's missing a cast)
>
> declare @i, @t;
>
> set @i = 0;
>
> while @i < 20
> begin
> set @t = 'aa' + @i;
> create table @t (id serial primary key, data text);
>
> set @i = @i + 1;
> end

I will look deeper at this problem tomorrow.

> - The corrected script gives no feedback that it's finished, other
> than re-enabling buttons. I would expect to see the appropriate
> notices from the server about each table that is created, and the
> status message on the status bar should change.

Right, it is the same feedback as Guillaume and I am currently working
on that. Once again it will be included in the next patch.

> - The following script (with missing increment of @i) gave appropriate
> errors when run the first time, but ran silently the second:
>
> declare @i, @t;
>
> set @i = 0;
>
> while @i < 20
> begin
> set @t = 'aa' + cast(@i as string);
> create table @t (id serial primary key, data text);
> end
>
> - Cancelling that script the first time round is awkward (we should
> offer a cancel option on the error dialogue). Using the Stop button
> seems to crash.

Weird, as the previous problem I need to look deeper at that in the
next following days.

About the documentation, I thought about adding information about the
pgScript button in the Query page and a link to the (possibly
reformated) syntax reference. About this syntax reference, how did you
find it? Was it clear?

Thank you for your feedback, it is really helpful!

Best regards,
Mickael