Re: wxWidgets 2.9 build

I'm having a go at getting pgadmin to build against the development
branch of wxWidgets, 2.9. I intend to first get a clean WxGTK build,
and perhaps move on to other WxWidgets platforms afterwards. I note
that the wxWidgets/contrib directory that is present in wxWidgets 2.8
that the various shell scripts and whatnot in ./xtra/wx-build cd into
and make within are absent from 2.9.

My understanding is that some of the contrib libraries made it to wx
core, while others were taken off to the wxCode add-ons website due to
the fact that they weren't being maintained.

I have some preliminary thoughts on how we should go about doing this.
I have attempted to run make against 2.9, while using the --ignore
flag to get some concept of the full extent of breakage, by grepping
through over 10 megabytes of errors.

The OGL (Object Graphics Library) contrib library, which we have a
dependency on, is missing. WxCode indicates that the now separately
distributed lib only supports 2.8. See
http://wxcode.sourceforge.net/showcomp.php?name=ogl . I will take a
run at getting this to work against 2.9, and failing that will ping
the maintainer and get his thoughts. Worryingly, wxCode says it's of
ALPHA status. I have downloaded it separately. The sourceforge
datestamp on ogl.tar.gz is '2007-03-28' - perhaps we should fork OGL
and maintain it in the pgadmin tree directly ourselves. I have some
concerns about the licencing though - apparently the wxWindows licence
is an LGPL variant, so doing this would, I believe, constitute
creating a derivative work, whereas we were merely "using" the work
before as defined by the licence.

I see a lot of ambiguous overload errors. Ambiguous
sysSettings::Write() and ctlListView::AppendItem() calls abound. This
looks like it's down to Wx 2.9's new unicode handling (see
http://docs.wxwidgets.org/trunk/overview_changes_since28.html).

I'll take another look tomorrow.

--
Regards,
Peter Geoghegan

Hi,

Le 15/01/2011 20:33, Peter Geoghegan a écrit :
> I'm having a go at getting pgadmin to build against the development
> branch of wxWidgets, 2.9. I intend to first get a clean WxGTK build,
> and perhaps move on to other WxWidgets platforms afterwards. I note
> that the wxWidgets/contrib directory that is present in wxWidgets 2.8
> that the various shell scripts and whatnot in ./xtra/wx-build cd into
> and make within are absent from 2.9.
>

If you get wxGTK to work, it should be easy to make it work on Windows
and Mac OS X.

> My understanding is that some of the contrib libraries made it to wx
> core, while others were taken off to the wxCode add-ons website due to
> the fact that they weren't being maintained.
>

+1

> I have some preliminary thoughts on how we should go about doing this.
> I have attempted to run make against 2.9, while using the --ignore
> flag to get some concept of the full extent of breakage, by grepping
> through over 10 megabytes of errors.
>
> The OGL (Object Graphics Library) contrib library, which we have a
> dependency on, is missing. WxCode indicates that the now separately
> distributed lib only supports 2.8. See
> http://wxcode.sourceforge.net/showcomp.php?name=ogl . I will take a
> run at getting this to work against 2.9, and failing that will ping
> the maintainer and get his thoughts. Worryingly, wxCode says it's of
> ALPHA status. I have downloaded it separately. The sourceforge
> datestamp on ogl.tar.gz is '2007-03-28' - perhaps we should fork OGL
> and maintain it in the pgadmin tree directly ourselves. I have some
> concerns about the licencing though - apparently the wxWindows licence
> is an LGPL variant, so doing this would, I believe, constitute
> creating a derivative work, whereas we were merely "using" the work
> before as defined by the licence.
>
> I see a lot of ambiguous overload errors. Ambiguous
> sysSettings::Write() and ctlListView::AppendItem() calls abound. This
> looks like it's down to Wx 2.9's new unicode handling (see
> http://docs.wxwidgets.org/trunk/overview_changes_since28.html).
>
> I'll take another look tomorrow.
>

I totally support this work. I really wish pgAdmin be compatible with
2.9. This will be a really hard work but a needed one.

But, just to make this perfectly clear right at the beginning, I'm
totally opposed to commit such a patch while wxWidgets 2.9/3.0 is not
available on Linux distros.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

I hope we can support both versions. 2.9 will be a great help on Mac
as we'll be able to use the Cocoa port.

On 1/15/11, Guillaume Lelarge <guillaume(at)lelarge(dot)info> wrote:
> Hi,
>
> Le 15/01/2011 20:33, Peter Geoghegan a écrit :
>> I'm having a go at getting pgadmin to build against the development
>> branch of wxWidgets, 2.9. I intend to first get a clean WxGTK build,
>> and perhaps move on to other WxWidgets platforms afterwards. I note
>> that the wxWidgets/contrib directory that is present in wxWidgets 2.8
>> that the various shell scripts and whatnot in ./xtra/wx-build cd into
>> and make within are absent from 2.9.
>>
>
> If you get wxGTK to work, it should be easy to make it work on Windows
> and Mac OS X.
>
>> My understanding is that some of the contrib libraries made it to wx
>> core, while others were taken off to the wxCode add-ons website due to
>> the fact that they weren't being maintained.
>>
>
> +1
>
>> I have some preliminary thoughts on how we should go about doing this.
>> I have attempted to run make against 2.9, while using the --ignore
>> flag to get some concept of the full extent of breakage, by grepping
>> through over 10 megabytes of errors.
>>
>> The OGL (Object Graphics Library) contrib library, which we have a
>> dependency on, is missing. WxCode indicates that the now separately
>> distributed lib only supports 2.8. See
>> http://wxcode.sourceforge.net/showcomp.php?name=ogl . I will take a
>> run at getting this to work against 2.9, and failing that will ping
>> the maintainer and get his thoughts. Worryingly, wxCode says it's of
>> ALPHA status. I have downloaded it separately. The sourceforge
>> datestamp on ogl.tar.gz is '2007-03-28' - perhaps we should fork OGL
>> and maintain it in the pgadmin tree directly ourselves. I have some
>> concerns about the licencing though - apparently the wxWindows licence
>> is an LGPL variant, so doing this would, I believe, constitute
>> creating a derivative work, whereas we were merely "using" the work
>> before as defined by the licence.
>>
>> I see a lot of ambiguous overload errors. Ambiguous
>> sysSettings::Write() and ctlListView::AppendItem() calls abound. This
>> looks like it's down to Wx 2.9's new unicode handling (see
>> http://docs.wxwidgets.org/trunk/overview_changes_since28.html).
>>
>> I'll take another look tomorrow.
>>
>
> I totally support this work. I really wish pgAdmin be compatible with
> 2.9. This will be a really hard work but a needed one.
>
> But, just to make this perfectly clear right at the beginning, I'm
> totally opposed to commit such a patch while wxWidgets 2.9/3.0 is not
> available on Linux distros.
>
>
> --
> Guillaume
> http://www.postgresql.fr
> http://dalibo.com
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Le 15/01/2011 20:52, Dave Page a écrit :
> I hope we can support both versions. 2.9 will be a great help on Mac
> as we'll be able to use the Cocoa port.
>

If it is possible, I cannot agree more with you. I'm afraid that the
patch will be huge and, as such, won't allow us to support both.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On 15 January 2011 20:18, Guillaume Lelarge <guillaume(at)lelarge(dot)info> wrote:
> If it is possible, I cannot agree more with you. I'm afraid that the
> patch will be huge and, as such, won't allow us to support both.

Is there any particular reason why you think that might be the case?
Switching from Carbon to Cocoa will be a separate patch. I don't own a
Mac, so someone else will have to write that.

An awful lot of the errors are in inline functions as it happens, so
we get to see them in every TU that the relevant header is included
in.

--
Regards,
Peter Geoghegan

The switch from carbon to cocoa shouldn't require any effort - thats
the work the wxWidgets guys have done.

On 1/15/11, Peter Geoghegan <peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> On 15 January 2011 20:18, Guillaume Lelarge <guillaume(at)lelarge(dot)info> wrote:
>> If it is possible, I cannot agree more with you. I'm afraid that the
>> patch will be huge and, as such, won't allow us to support both.
>
> Is there any particular reason why you think that might be the case?
> Switching from Carbon to Cocoa will be a separate patch. I don't own a
> Mac, so someone else will have to write that.
>
> An awful lot of the errors are in inline functions as it happens, so
> we get to see them in every TU that the relevant header is included
> in.
>
> --
> Regards,
> Peter Geoghegan
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 15 January 2011 21:26, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> The switch from carbon to cocoa shouldn't require any effort - thats
> the work the wxWidgets guys have done.

Sure, but I would have imagined that there might be one or two small
things to clean up.

I've whittled down the amount of output from make to a mere 250kb.
Now, most translation units compile:

[peter(at)localhost pgadmin]$ ls *.o
ctlCheckTreeView.o dlgRule.o gqbView.o
pgFunction.o pgsNegate.o
ctlCodeWindow.o dlgSchedule.o
gqbViewPanels.o pgGroup.o pgsNot.o
ctlColourPicker.o dlgSchema.o keywords.o
pgIndexConstraint.o pgsNumberGen.o
ctlComboBox.o dlgSelectDatabase.o lex.pgs.o
pgLanguage.o pgsNumber.o
ctlDefaultSecurityPanel.o dlgSequence.o macros.o
pgOperatorClass.o pgsObjectGen.o
ctlMenuToolbar.o dlgServer.o mapm5sin.o
pgOperatorFamily.o pgsOperation.o
ctlMessageWindow.o dlgStep.o mapm_add.o
pgOperator.o pgsOr.o
ctlResultGrid.o dlgSynonym.o mapmasin.o
pgRule.o pgsOver.o
ctlSecurityPanel.o dlgTable.o mapmasn0.o
pgsAlloc.o pgsParameterException.o
ctlSQLGrid.o dlgTablespace.o mapmcbrt.o
pgsAnd.o pgsParenthesis.o
ctlSQLResult.o dlgTextSearchConfiguration.o mapmcnst.o
pgsArithmeticException.o pgsPlus.o
ctlStackWindow.o dlgTextSearchDictionary.o mapm_cpi.o
pgsAssertException.o pgsPrintStmt.o
ctlTabWindow.o dlgTextSearchParser.o mapm_div.o
pgsAssertStmt.o pgsProgram.o
ctlTree.o dlgTextSearchTemplate.o mapm_exp.o
pgsAssign.o pgsRealGen.o
ctlVarWindow.o dlgTrigger.o mapmfact.o
pgsAssignToRecord.o pgsRecord.o
dbgBreakPoint.o dlgType.o mapm_fam.o
pgsBreakException.o pgsRemoveLine.o
dbgDbResult.o dlgUser.o mapm_fft.o
pgsBreakStmt.o pgsStmtList.o
dbgPgThread.o edbPackageFunction.o mapm_flr.o
pgsCastException.o pgsStmt.o
dbgResultset.o edbPackage.o mapmfmul.o
pgsCast.o pgsStringGen.o
dbgTargetInfo.o edbPackageVariable.o mapm_fpf.o
pgSchema.o pgsString.o
debugger.o edbPrivateSynonym.o mapm_gcd.o
pgsColumns.o pgsThread.o
dlgAddFavourite.o edbSynonym.o mapmgues.o
pgsContinueException.o pgsTimeGen.o
dlgAggregate.o events.o mapmhasn.o
pgsContinueStmt.o pgsTimes.o
dlgCast.o factory.o mapmhsin.o
pgsDateGen.o pgsTrim.o
dlgCheck.o favourites.o mapmipwr.o
pgsDateTimeGen.o pgsUtilities.o
dlgClasses.o frmAbout.o mapmistr.o
pgsDeclareRecordStmt.o pgsVariable.o
dlgColumn.o frmBackupGlobals.o mapm_lg2.o
pgsDifferent.o pgsWhileStmt.o
dlgConnect.o frmBackup.o mapm_lg3.o
pgsDriver.o pgTable.o
dlgConversion.o frmBackupServer.o mapm_lg4.o
pgsEqual.o pgTablespace.o
dlgDatabase.o frmConfig.o mapm_log.o
pgSequence.o pgTextSearchConfiguration.o
dlgDirectDbg.o frmDebugger.o mapm_mul.o
pgServer.o pgTextSearchDictionary.o
dlgDomain.o frmExport.o mapm_pow.o
pgSet.o pgTextSearchParser.o
dlgEditGridOptions.o frmGrantWizard.o mapmpwr2.o
pgsException.o pgTextSearchTemplate.o
dlgFindReplace.o frmMain.o mapm_rcp.o
pgsExecute.o pgTrigger.o
dlgForeignKey.o frmMaintenance.o mapm_rnd.o
pgsExpression.o pgType.o
dlgFunction.o frmOptions.o mapmrsin.o
pgsExpressionStmt.o pgUser.o
dlgGroup.o frmPassword.o mapm_set.o
pgsGenDate.o pgView.o
dlgHbaConfig.o frmRestore.o mapm_sin.o
pgsGenDateTime.o plugins.o
dlgIndexConstraint.o frmSplash.o mapmsqrt.o
pgsGenDictionary.o registry.o
dlgIndex.o gpExtTable.o mapmstck.o
pgsGenerator.o slCluster.o
dlgJob.o gpPartition.o mapmutil.o
pgsGenInt.o slListen.o
dlgLanguage.o gpResQueue.o mapmutl1.o
pgsGenReal.o slNode.o
dlgMainConfig.o gqbArrayCollection.o mapmutl2.o
pgsGenReference.o slPath.o
dlgManageFavourites.o gqbBrowser.o misc.o
pgsGenRegex.o slSequence.o
dlgManageMacros.o gqbCollection.o pgAggregate.o
pgsGenString.o slSubscription.o
dlgOperator.o gqbColumn.o pgaJob.o
pgsGenTime.o slTable.o
dlgPackage.o gqbController.o pgaSchedule.o
pgsGreaterEqual.o sysLogger.o
dlgPgpassConfig.o gqbDatabase.o pgaStep.o
pgsGreater.o sysProcess.o
dlgProperty.o gqbGraphSimple.o pgCast.o
pgsIdent.o sysSettings.o
dlgReassignDropOwned.o gqbGridJoinTable.o
pgCatalogObject.o pgsIdentRecord.o tabcomplete.o
dlgRepListen.o gqbGridOrderTable.o pgCheck.o
pgsIfStmt.o utffile.o
dlgRepNode.o gqbGridProjTable.o
pgCollection.o pgsIntegerGen.o xh_calb.o
dlgRepPath.o gqbGridRestTable.o pgColumn.o
pgsInterruptException.o xh_ctlchecktreeview.o
dlgRepProperty.o gqbModel.o
pgConstraints.o pgsLines.o xh_ctlcolourpicker.o
dlgRepSequence.o gqbObjectCollection.o
pgConversion.o pgsLowerEqual.o xh_ctlcombo.o
dlgRepSet.o gqbObject.o pgDatabase.o
pgsLower.o xh_ctltree.o
dlgRepSubscription.o gqbQueryObjs.o pgDatatype.o
pgsMapm.o xh_sqlbox.o
dlgRepTable.o gqbSchema.o pgDomain.o
pgsMinus.o xh_timespin.o
dlgRole.o gqbTable.o
pgForeignKey.o pgsModulo.o xrcDialogs.o

The main culprit that breaks compatibility here seems to be multiple
conversion operators in wxString, in particular the addition of a new
operator const void*(). This is apparently used as a hack to get
greater type safety by creating an ambiguity that prevents code from
compiling that relies on questionable implicit casts. If I came up
with this, I definitely would have called it "the three stooges
idiom". Conversion operators are a bit frowned upon, and it seems a
bit strange that wxString has both a conversion operator and a
std::string style c_str() (in fact, multiple variants of both), but I
guess it's a legacy thing.

Why does sysSettings::Write(const wxString&, const wxChar*) take a
const wxChar* as its second argument rather than just a wxString? I'm
inclined to just use a const wxString&, and avoid c strings if at all
possible, which resolves the ambiguity in various places. Obviously we
can't have null references, so I'm going to write workarounds where
that was possible before, where there might have been different
codepaths for NULL and so on.

I'm changing our use of wxCalendarCtrl to wxGenericCalendarCtrl where
required but not where unavailable. We won't get 2.9's new native GTK
calendar controls on that platform, but I don't suppose it matters too
much. Nothing changes, and nothing is broken. If we could afford to
have the GTK calendar, it wouldn't have broken our code, because its
interface is a subset of the old wxCalendarCtrl/current
wxGenericCalendarCtrl interface. I'll write a typedef called
wxCompatCalendar for either wxCalendarCtrl or wxGenericCalendarCtrl,
depending on a preprocessor macro wxCHECK_VERSION(2, 9, 0) that
indicates Wx version is 2.9+. This is sort of like WxChar (which can
be char or wchar_t, depending on whether or not we're doing a unicode
build or an ANSI build). I'm still working on this.

in pgsDictionaryGen.cpp, why do we do this?:

wxString line;
while ((line = text.ReadLine()) && !input.Eof())
{
++result;
}

We used to implicitly cast to WxChar*, but the ambiguity now occurs.
It looks like the three stooges idiom has worked in our favour here.
This piece of code looks broken, like someone has made the classic
mistake of using an assignment operator in place of a comparison
operator . Please comment.

I saw see lots and lots of errors like this:

../pgadmin/include/utils/sysLogger.h:52:1: error: expected initializer
before ‘ATTRIBUTE_PRINTF_1’

ATTRIBUTE_PRINTF_1 isn't #define'd. It's called WX_ATTRIBUTE_PRINTF_1
is 2.9, so I've written a new macro called COMPAT_ATTRIBUTE_PRINTF_1
that is conditionally #define'd as either variant.

I'm going to go out now. I'll take another look tomorrow. It looks
like supporting both 2.8 and 2.9 is quite possible.

--
Regards,
Peter Geoghegan

On Sun, Jan 16, 2011 at 6:47 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> On 15 January 2011 21:26, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> The switch from carbon to cocoa shouldn't require any effort - thats
>> the work the wxWidgets guys have done.
>
> Sure, but I would have imagined that there might be one or two small
> things to clean up.

That's not problem. I thought you were implying large changes might be
necessary, which should not be the case.

> I've whittled down the amount of output from make to a mere 250kb.

Nice.

> The main culprit that breaks compatibility here seems to be multiple
> conversion operators in wxString, in particular the addition of a new
> operator const void*(). This is apparently used as a hack to get
> greater type safety by creating an ambiguity that prevents code from
> compiling that relies on questionable implicit casts. If I came up
> with this, I definitely would have called it "the three stooges
> idiom". Conversion operators are a bit frowned upon, and it seems a
> bit strange that wxString has both a conversion operator and a
> std::string style c_str() (in fact, multiple variants of both), but I
> guess it's a legacy thing.

From what I can see in a quick glance at the docs, the only conversion
operator is to const char* in 2.8, so I guess that is a legacy thing
as you suggest. There are good reasons for the different conversion
members like c_str() and mb_str().

> Why does sysSettings::Write(const wxString&, const wxChar*) take a
> const wxChar* as its second argument rather than just a wxString? I'm
> inclined to just use a const wxString&, and avoid c strings if at all
> possible, which resolves the ambiguity in various places. Obviously we
> can't have null references, so I'm going to write workarounds where
> that was possible before, where there might have been different
> codepaths for NULL and so on.

Excellent question - especially as it just calls wxConfig::Write,
which is basically "bool wxConfigBase::Write(const wxString& key,
const wxString& value)"

So unless I'm missing something, it can just be changed.

> I'm changing our use of wxCalendarCtrl to wxGenericCalendarCtrl where
> required but not where unavailable. We won't get 2.9's new native GTK
> calendar controls on that platform, but I don't suppose it matters too
> much. Nothing changes, and nothing is broken. If we could afford to
> have the GTK calendar, it wouldn't have broken our code, because its
> interface is a subset of the old wxCalendarCtrl/current
> wxGenericCalendarCtrl interface. I'll write a typedef called
> wxCompatCalendar for either wxCalendarCtrl or wxGenericCalendarCtrl,
> depending on a preprocessor macro wxCHECK_VERSION(2, 9, 0) that
> indicates Wx version is 2.9+. This is sort of like WxChar (which can
> be char or wchar_t, depending on whether or not we're doing a unicode
> build or an ANSI build). I'm still working on this.

OK.

> in pgsDictionaryGen.cpp, why do we do this?:
>
>                        wxString line;
>                        while ((line = text.ReadLine()) && !input.Eof())
>                        {
>                                ++result;
>                        }
>
> We used to implicitly cast to WxChar*, but the ambiguity now occurs.
> It looks like the three stooges idiom has worked in our favour here.
> This piece of code looks broken, like someone has made the classic
> mistake of using an assignment operator in place of a comparison
> operator . Please comment.

It's getting late and I've had an extremely stressful weekend, so
maybe I'm missing something obvious but I don't see an issue there.
It's reading from the stream (text) line by line into a dummy variable
(line), and counting the lines until it gets to the end of the file
(input).

> I saw see lots and lots of errors like this:
>
> ../pgadmin/include/utils/sysLogger.h:52:1: error: expected initializer
> before ‘ATTRIBUTE_PRINTF_1’
>
> ATTRIBUTE_PRINTF_1 isn't #define'd. It's called WX_ATTRIBUTE_PRINTF_1
> is 2.9, so I've written a new macro called COMPAT_ATTRIBUTE_PRINTF_1
> that is conditionally #define'd as either variant.

OK.

> I'm going to go out now. I'll take another look tomorrow. It looks
> like supporting both 2.8 and 2.9 is quite possible.

Sounds like it.

How did the OGL port go? I looked at that briefly, and had a rough
build in 10 minutes or so iirc. Oh, and in answer to your previous
comments on the distribution; you're quite correct - the wxWidgets
licence isn't suitable for distribution with pgAdmin. We'd have to get
the patch applied upstream, or host our own port. If we need to do the
latter, then I guess we should also setup our own GIT repo.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 16 January 2011 22:39, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> On Sun, Jan 16, 2011 at 6:47 PM, Peter Geoghegan
> From what I can see in a quick glance at the docs, the only conversion
> operator is to const char* in 2.8, so I guess that is a legacy thing
> as you suggest. There are good reasons for the different conversion
> members like c_str() and mb_str().

I meant that the co-existence of both implicit conversions and
redundant explicit functions must be a legacy issue. I appreciate that
there are good reasons for the variants like mb_str() though. In case
you didn't get the joke, I called it the three stooges idiom because
they used to attempt to all get through a door threshold at the same
time and block each other out.

>> Why does sysSettings::Write(const wxString&, const wxChar*) take a
>> const wxChar* as its second argument rather than just a wxString? I'm
>> inclined to just use a const wxString&, and avoid c strings if at all
>> possible, which resolves the ambiguity in various places. Obviously we
>> can't have null references, so I'm going to write workarounds where
>> that was possible before, where there might have been different
>> codepaths for NULL and so on.
>
> Excellent question - especially as it just calls wxConfig::Write,
> which is basically "bool wxConfigBase::Write(const wxString& key,
> const wxString& value)"
>
> So unless I'm missing something, it can just be changed.

Good. Already done.

>> in pgsDictionaryGen.cpp, why do we do this?:
>>
>>                        wxString line;
>>                        while ((line = text.ReadLine()) && !input.Eof())
>>                        {
>>                                ++result;
>>                        }
>>

> It's getting late and I've had an extremely stressful weekend, so
> maybe I'm missing something obvious but I don't see an issue there.
> It's reading from the stream (text) line by line into a dummy variable
> (line), and counting the lines until it gets to the end of the file
> (input).

Sorry to hear that. I just thought that it might not be prudent to
rely on the implicit cast, and that it superficially looked like the
classic "assignment operator in place of equality operator" mistake.
Perhaps I should have been clearer on that.

> How did the OGL port go? I looked at that briefly, and had a rough
> build in 10 minutes or so iirc. Oh, and in answer to your previous
> comments on the distribution; you're quite correct - the wxWidgets
> licence isn't suitable for distribution with pgAdmin. We'd have to get
> the patch applied upstream, or host our own port. If we need to do the
> latter, then I guess we should also setup our own GIT repo.

I didn't look at it yet. I thought that it would be better to leave it
until there was a consensus on how to deal with it, and just tackle
the landslide of other compiler errors. Hopefully the fact that
compilation of certain TUs is terminated when ogl.h can't be included
isn't masking a whole lot more errors at this point. Have we
considered how maintaining OGL ourselves will affect package
maintainers and so on?

IANAL, but it seems like a separate git repo is the way to go to
protect ourselves from copyleft implications. It might be worth asking
the author/maintainer to change the licence. That's highly unlikely to
happen, but it's still worth asking the question.

We're down to just 25kb of make output when I haven't done a make
clean first; otherwise, there'd be some additional warnings which I'll
fix later. Plus, as I said, I still haven't looked at OGL, and we can
take it that once we've dealt with that issue, more unrelated errors
will occur as we get through those additional TUs.

The fact that wxString::c_str() now returns a special proxy object is
a pain. Because it doesn't have a compiler generated copy constructor
or is composed of objects that don't, you cannot pass it to vararg
functions, of which there seems to be a few. I'm cleaning that up by
explicitly casting to const wxChar* (and generally not calling c_str()
and not going through the proxy object). That seems to work fine.
However, it's not immediately clear how to do this in the bison
grammar file pgsParser.yy; changing something like this:

@@ -886,8 +886,8 @@ assign_element
| PGS_IDENTIFIER '[' expression ']' '[' expression ']'
PGS_EQ_OP expression
{

wxLogScriptVerbose(wxT("SET %s[%s][%s] = %s"),
-
$1->c_str(), $3->value().c_str(),
-
$6->value().c_str(), $9->value().c_str());
+
((const wxChar*) $1), ((const wxChar*) $3->value()),
+
((const wxChar*) $6->value()), ((const wxChar*)
$9->value()));

doesn't fix the problem. I still see the same error, like
"pgscript/pgsParser.yy:494:92: error: cannot pass objects of
non-trivially-copyable type ‘class wxCStrData’ through ‘...’".

This code in ctlSQLBox::RegexFindText() errors:

wxWX2MBbuf buf = (wxWX2MBbuf)wx2stc(text);

wx2stc is now in private.h . See
http://groups.google.com/group/wx-dev/browse_thread/thread/1477c44701f792ea/8205f32f8433b03b
and please advise.

For your information, I've attached the text of remaining errors. A
few mysteries remain, such as why some classes that we were
instantiating have become abstract. I haven't really looked into that
yet. I'm going to give OGL some attention now.

--
Regards,
Peter Geoghegan

Le 17/01/2011 16:30, Peter Geoghegan a écrit :
> On 16 January 2011 22:39, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> On Sun, Jan 16, 2011 at 6:47 PM, Peter Geoghegan
> [...]
>> How did the OGL port go? I looked at that briefly, and had a rough
>> build in 10 minutes or so iirc. Oh, and in answer to your previous
>> comments on the distribution; you're quite correct - the wxWidgets
>> licence isn't suitable for distribution with pgAdmin. We'd have to get
>> the patch applied upstream, or host our own port. If we need to do the
>> latter, then I guess we should also setup our own GIT repo.
>
> I didn't look at it yet. I thought that it would be better to leave it
> until there was a consensus on how to deal with it, and just tackle
> the landslide of other compiler errors. Hopefully the fact that
> compilation of certain TUs is terminated when ogl.h can't be included
> isn't masking a whole lot more errors at this point. Have we
> considered how maintaining OGL ourselves will affect package
> maintainers and so on?
>

Actually, I don't see us maintaining OGL. We don't have the manpower to
do that.

I'm wondering if we really need OGL. What do we use it for? because if
it's now out of wxWidgets and if it's an important component for us, we
aren't probably alone. What do the other guys do? Moreover, if they got
rid of it, it's also probably because they don't need it, meaning we
could use something else, available in wxWidgets 2.9 that would replace
OGL. That would be a really better way than maintaining a fork of OGL.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Mon, Jan 17, 2011 at 3:30 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
>>> in pgsDictionaryGen.cpp, why do we do this?:
>>>
>>>                        wxString line;
>>>                        while ((line = text.ReadLine()) && !input.Eof())
>>>                        {
>>>                                ++result;
>>>                        }
>>>
>
>> It's getting late and I've had an extremely stressful weekend, so
>> maybe I'm missing something obvious but I don't see an issue there.
>> It's reading from the stream (text) line by line into a dummy variable
>> (line), and counting the lines until it gets to the end of the file
>> (input).
>
> Sorry to hear that. I just thought that it might not be prudent to
> rely on the implicit cast, and that it superficially looked like the
> classic "assignment operator in place of equality operator" mistake.
> Perhaps I should have been clearer on that.

Oh, I see what you mean. I was staring right through those additional
brackets :-). I'd break it out for clarity.

>> How did the OGL port go? I looked at that briefly, and had a rough
>> build in 10 minutes or so iirc. Oh, and in answer to your previous
>> comments on the distribution; you're quite correct - the wxWidgets
>> licence isn't suitable for distribution with pgAdmin. We'd have to get
>> the patch applied upstream, or host our own port. If we need to do the
>> latter, then I guess we should also setup our own GIT repo.
>
> I didn't look at it yet. I thought that it would be better to leave it
> until there was a consensus on how to deal with it, and just tackle
> the landslide of other compiler errors. Hopefully the fact that
> compilation of certain TUs is terminated when ogl.h can't be included
> isn't masking a whole lot more errors at this point. Have we
> considered how maintaining OGL ourselves will affect package
> maintainers and so on?

It'll annoy them for sure, but we don't really have any choice unless
we want to rip out the graphical explain tool, and query builder :-(

> IANAL, but it seems like a separate git repo is the way to go to
> protect ourselves from copyleft implications. It might be worth asking
> the author/maintainer to change the licence. That's highly unlikely to
> happen, but it's still worth asking the question.

Yeah - I imagine the code had a few contributors over the years. The
wxWidgets licence is a weird one - bizarrely, it allows closed source
developers to incorporate the code in their source trees, but not open
source guys using liberal licences like ours.

> We're down to just 25kb of make output when I haven't done a make
> clean first; otherwise, there'd be some additional warnings which I'll
> fix later. Plus, as I said, I still haven't looked at OGL, and we can
> take it that once we've dealt with that issue, more unrelated errors
> will occur as we get through those additional TUs.

:-)

> The fact that wxString::c_str() now returns a special proxy object is
> a pain. Because it doesn't have a compiler generated copy constructor
> or is composed of objects that don't, you cannot pass it to vararg
> functions, of which there seems to be a few. I'm cleaning that up by
> explicitly casting to const wxChar* (and generally not calling c_str()
> and not going through the proxy object). That seems to work fine.

I imagine that makes for a big (and ugly) patch - the main reason we
use c_str() is so we can pass wxStrings to variadics.

> However, it's not immediately clear how to do this in the bison
> grammar file pgsParser.yy; changing something like this:
>
> @@ -886,8 +886,8 @@ assign_element
>        | PGS_IDENTIFIER '[' expression ']' '[' expression ']'
> PGS_EQ_OP expression
>                                                                {
>
>  wxLogScriptVerbose(wxT("SET %s[%s][%s] = %s"),
> -
>                 $1->c_str(), $3->value().c_str(),
> -
>                 $6->value().c_str(), $9->value().c_str());
> +
>                  ((const wxChar*) $1), ((const wxChar*) $3->value()),
> +
>                  ((const wxChar*) $6->value()), ((const wxChar*)
> $9->value()));
>
> doesn't fix the problem. I still see the same error, like
> "pgscript/pgsParser.yy:494:92: error: cannot pass objects of
> non-trivially-copyable type ‘class wxCStrData’ through ‘...’".

I've never even looked at that code I don't think.

> This code in  ctlSQLBox::RegexFindText() errors:
>
> wxWX2MBbuf buf = (wxWX2MBbuf)wx2stc(text);
>
> wx2stc is now in private.h . See
> http://groups.google.com/group/wx-dev/browse_thread/thread/1477c44701f792ea/8205f32f8433b03b
> and please advise.

Hmm, I need to look into that some more when I'm a little more awake.
Probably not for a couple of days though unfortunately.

> For your information, I've attached the text of remaining errors. A
> few mysteries remain, such as why some classes that we were
> instantiating have become abstract. I haven't really looked into that
> yet. I'm going to give OGL some attention now.

OK - thanks!

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan 17, 2011 at 10:13 PM, Guillaume Lelarge
<guillaume(at)lelarge(dot)info> wrote:
> Actually, I don't see us maintaining OGL. We don't have the manpower to
> do that.
>
> I'm wondering if we really need OGL. What do we use it for? because if
> it's now out of wxWidgets and if it's an important component for us, we
> aren't probably alone. What do the other guys do? Moreover, if they got
> rid of it, it's also probably because they don't need it, meaning we
> could use something else, available in wxWidgets 2.9 that would replace
> OGL. That would be a really better way than maintaining a fork of OGL.

We use it for drawing graphical query plans, and I believe for the
query builder. Replacing it would probably not be straightforward.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Le 17/01/2011 23:20, Dave Page a écrit :
> On Mon, Jan 17, 2011 at 10:13 PM, Guillaume Lelarge
> <guillaume(at)lelarge(dot)info> wrote:
>> Actually, I don't see us maintaining OGL. We don't have the manpower to
>> do that.
>>
>> I'm wondering if we really need OGL. What do we use it for? because if
>> it's now out of wxWidgets and if it's an important component for us, we
>> aren't probably alone. What do the other guys do? Moreover, if they got
>> rid of it, it's also probably because they don't need it, meaning we
>> could use something else, available in wxWidgets 2.9 that would replace
>> OGL. That would be a really better way than maintaining a fork of OGL.
>
> We use it for drawing graphical query plans, and I believe for the
> query builder. Replacing it would probably not be straightforward.
>

And we sure cannot get rid of the graphical query plans and the
graphical query builder. Guess you're right.

Well, the good thing seems my translation work is done, so i guess I'm
now full-spare-time on pgAdmin again. Many things on my plate for the
next days (some bugs to fix, new options dialog, custom reports in the
status window, new PostgreSQL features, etc). Nothing related to this
thread obviously, but I'm glad to be back :)

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

Hmmm....seem to be getting a weird linker error when I build OGL (from
the independent package) against 2.9:

/usr/bin/ld: ogl_dll_composit.o: relocation R_X86_64_32 against
`wxDivisionControlPoint::ms_classInfo' can not be used when making a
shared object; recompile with -fPIC
ogl_dll_composit.o: could not read symbols: Bad value
collect2: ld returned 1 exit status

I take it that you didn't see this Dave? I built it against 2.9, which
was itself built using a shell script that's the same as
./xtra/build-wxgtk, except that I removed the contrib stuff.

Should -fPIC be in our CXXFLAGS? If so, why wouldn't it be?

> Yeah - I imagine the code had a few contributors over the years. The
> wxWidgets licence is a weird one - bizarrely, it allows closed source
> developers to incorporate the code in their source trees, but not open
> source guys using liberal licences like ours.

I'll ask the question so.

> I imagine that makes for a big (and ugly) patch - the main reason we
> use c_str() is so we can pass wxStrings to variadics.

Maybe we should consider using an alternative to conventional variadic
functions. There are lots of things to not like about them.
Boost.format is a good alternative . It may not actually be worth
pursuing, but it's worth noting.

>> This code in  ctlSQLBox::RegexFindText() errors:
>>
>> wxWX2MBbuf buf = (wxWX2MBbuf)wx2stc(text);
>>
>> wx2stc is now in private.h . See
>> http://groups.google.com/group/wx-dev/browse_thread/thread/1477c44701f792ea/8205f32f8433b03b
>> and please advise.
>
> Hmm, I need to look into that some more when I'm a little more awake.
> Probably not for a couple of days though unfortunately.

Find attached latest error output. All miscellaneous issues have been
resolved. I'm only missing these object files now:

g++: pgAdmin3.o: No such file or directory
g++: ctlSQLBox.o: No such file or directory
g++: explainCanvas.o: No such file or directory
g++: explainShape.o: No such file or directory
g++: frmQuery.o: No such file or directory
g++: parser.tab.o: No such file or directory

I believe that the only things standing between us and a rough 2.9 build are:

1. The linker problem above. OGL compiles acceptably.
2. The problem with pgsParser.yy. This is probably exceedingly simple,
but I haven't managed to fix it yet.
3. The problem with wx2stc() above. I could have fixed this by copying
and pasting the tiny function which only appears in private.h in 2.9,
but it that would violate the wxWindows licence.

I have had to do one or two things that I'm not overjoyed with to get
to this point, including using a const_cast to cast away constness
from a handle (wxChar*) to the internal buffer used by wxString.

--
Regards,
Peter Geoghegan

On Mon, Jan 17, 2011 at 11:56 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> Hmmm....seem to be getting a weird linker error when I build OGL (from
> the independent package) against 2.9:
>
> /usr/bin/ld: ogl_dll_composit.o: relocation R_X86_64_32 against
> `wxDivisionControlPoint::ms_classInfo' can not be used when making a
> shared object; recompile with -fPIC
> ogl_dll_composit.o: could not read symbols: Bad value
> collect2: ld returned 1 exit status
>
> I take it that you didn't see this Dave? I built it against 2.9, which
> was itself built using a shell script that's the same as
> ./xtra/build-wxgtk, except that I removed the contrib stuff.

No, I didn't. FYI, I was building on Mac which tends to have it's own
unique set of issues due to the Universal Binaries the platform uses.

> Should -fPIC be in our CXXFLAGS? If so, why wouldn't it be?

I don't know - should it? We're not building any shared libraries, so
does it matter?

That reminds me of another issue that needs fixing at some point; the
build system confuses CPPFLAGS and CXXFLAGS.

>> I imagine that makes for a big (and ugly) patch - the main reason we
>> use c_str() is so we can pass wxStrings to variadics.
>
> Maybe we should consider using an alternative to conventional variadic
> functions. There are lots of things to not like about them.
> Boost.format is a good alternative . It may not actually be worth
> pursuing, but it's worth noting.

That would be a huge patch - and it would need some serious
justification for us to consider moving away from the wxWidgets way of
doing things.

> Find attached latest error output. All miscellaneous issues have been
> resolved. I'm only missing these object files now:
>
> g++: pgAdmin3.o: No such file or directory
> g++: ctlSQLBox.o: No such file or directory
> g++: explainCanvas.o: No such file or directory
> g++: explainShape.o: No such file or directory
> g++: frmQuery.o: No such file or directory
> g++: parser.tab.o: No such file or directory
>
> I believe that the only things standing between us and a rough 2.9 build are:
>
> 1. The linker problem above. OGL compiles acceptably.

That'll probably fix explainCanvas and explainShape.

> 2. The problem with pgsParser.yy. This is probably exceedingly simple,
> but I haven't managed to fix it yet.
> 3. The problem with wx2stc() above. I could have fixed this by copying
> and pasting the tiny function which only appears in private.h in 2.9,
> but it that would violate the wxWindows licence.

<grumble>. So we need to convert from a wxString to a UCS2 char array.
http://docs.wxwidgets.org/stable/wx_wxmbconvutf16.html#wxmbconvutf16
seems like it might do the trick.

> I have had to do one or two things that I'm not overjoyed with to get
> to this point, including using a const_cast to cast away constness
> from a handle (wxChar*) to the internal buffer used by wxString.

Urgh.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 18 January 2011 09:31, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> I take it that you didn't see this Dave? I built it against 2.9, which
>> was itself built using a shell script that's the same as
>> ./xtra/build-wxgtk, except that I removed the contrib stuff.
>
> No, I didn't. FYI, I was building on Mac which tends to have it's own
> unique set of issues due to the Universal Binaries the platform uses.
>
>> Should -fPIC be in our CXXFLAGS? If so, why wouldn't it be?
>
> I don't know - should it? We're not building any shared libraries, so
> does it matter?

I meant for OGL.

>> I believe that the only things standing between us and a rough 2.9 build are:
>>
>> 1. The linker problem above. OGL compiles acceptably.
>
> That'll probably fix explainCanvas and explainShape.

I've e-mailed the original developer, so hopefully he'll be able to
help on my linker problem. I asked the listed maintainer about the
licensing thing, and I may have been wrong to be skeptical. Changing
the licensing looks like something that could actually happen.

>> 3. The problem with wx2stc() above. I could have fixed this by copying
>> and pasting the tiny function which only appears in private.h in 2.9,
>> but it that would violate the wxWindows licence.
>
>
> <grumble>. So we need to convert from a wxString to a UCS2 char array.
> http://docs.wxwidgets.org/stable/wx_wxmbconvutf16.html#wxmbconvutf16
> seems like it might do the trick.

It seems to just be used in wxStyledTextCtrl, which was contrib in
2.8. Now it's mainstream. We always used it though. Anyway, the reason
we don't use what you've linked to, is, I suppose, the same reason why
they don't in PlatWX.cpp where unicode variants of wx2stc() are
defined:

// Convert using Scintilla's functions instead of wx's, Scintilla's are more
// forgiving and won't assert...

>> I have had to do one or two things that I'm not overjoyed with to get
>> to this point, including using a const_cast to cast away constness
>> from a handle (wxChar*) to the internal buffer used by wxString.
>
> Urgh.

I just meant that there are one or two times that happens as a
temporary stopgap. It may not have been worth mentioning, as I expect
that they'll be fixed before this patch is reviewed. They aren't
pervasive, because the questionable practice of manipulating a
wxString buffer through a wxChar* isn't pervasive in the existing
code. I currently do the right thing in the vast majority of cases,
though the right thing for portability as documented by "Potential
Unicode Pitfalls" is often a bit on the ugly side.

Trying to get OGL built now. Makefile seems to have been made with a
fairly old version of bakefile. I think at the very least, we should
be able to get it officially supported with 2.9.

--
Regards,
Peter Geoghegan

On Tue, Jan 18, 2011 at 2:51 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> On 18 January 2011 09:31, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>> I take it that you didn't see this Dave? I built it against 2.9, which
>>> was itself built using a shell script that's the same as
>>> ./xtra/build-wxgtk, except that I removed the contrib stuff.
>>
>> No, I didn't. FYI, I was building on Mac which tends to have it's own
>> unique set of issues due to the Universal Binaries the platform uses.
>>
>>> Should -fPIC be in our CXXFLAGS? If so, why wouldn't it be?
>>
>> I don't know - should it? We're not building any shared libraries, so
>> does it matter?
>
> I meant for OGL.

Oh, oops. Probably in that case - we should be using whatever
wxWidgets does really though.

>>> I believe that the only things standing between us and a rough 2.9 build are:
>>>
>>> 1. The linker problem above. OGL compiles acceptably.
>>
>> That'll probably fix explainCanvas and explainShape.
>
> I've e-mailed the original developer, so hopefully he'll be able to
> help on my linker problem. I asked the listed maintainer about the
> licensing thing, and I may have been wrong to be skeptical. Changing
> the licensing looks like something that could actually happen.

Oh, cool.

>>> 3. The problem with wx2stc() above. I could have fixed this by copying
>>> and pasting the tiny function which only appears in private.h in 2.9,
>>> but it that would violate the wxWindows licence.
>>
>>
>> <grumble>. So we need to convert from a wxString to a UCS2 char array.
>> http://docs.wxwidgets.org/stable/wx_wxmbconvutf16.html#wxmbconvutf16
>> seems like it might do the trick.
>
> It seems to just be used in wxStyledTextCtrl, which was contrib in
> 2.8. Now it's mainstream. We always used it though. Anyway, the reason
> we don't use what you've linked to, is, I suppose, the same reason why
> they don't in PlatWX.cpp where unicode variants of wx2stc() are
> defined:
>
> // Convert using Scintilla's functions instead of wx's, Scintilla's are more
> // forgiving and won't assert...

I'm really not sure that's an issue for us. Postgres is already strict
about UTF-8 - I don't see why we shouldn't be either (especially as
this only affects regexp searching). Unless it's asserting about
things other than invalid chars or other real problems?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hmmm....I've come up against a problem.

I found that when I ran the shell script parser.sh, bison and flex
were run on the pgscript context-free grammar. My casts seem to work.
However, a new problem has emerged. Here's part of the make output:

g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
lex.pgs.o -MD -MP -MF .deps/lex.pgs.Tpo -c -o lex.pgs.o `test -f
'./pgscript/lex.pgs.cc' || echo './'`./pgscript/lex.pgs.cc
pgscript/lex.pgs.cc:315:25: error: no ‘int pgsFlexLexer::yywrap()’
member function declared in class ‘pgsFlexLexer’
In file included from pgscript/pgsScanner.ll:17:0:
pgscript/pgsParser.yy:109:2: error: ‘pgsExpression’ does not name a type
pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
make[2]: [lex.pgs.o] Error 1 (ignored)
mv -f .deps/lex.pgs.Tpo .deps/lex.pgs.Po
mv: cannot stat `.deps/lex.pgs.Tpo': No such file or directory
make[2]: [lex.pgs.o] Error 1 (ignored)
g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
parser.tab.o -MD -MP -MF .deps/parser.tab.Tpo -c -o parser.tab.o `test
-f './pgscript/parser.tab.cc' || echo './'`./pgscript/parser.tab.cc
mv -f .deps/parser.tab.Tpo .deps/parser.tab.Po
g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
pgsCast.o -MD -MP -MF .deps/pgsCast.Tpo -c -o pgsCast.o `test -f
'./pgscript/expressions/pgsCast.cpp' || echo
'./'`./pgscript/expressions/pgsCast.cpp
In file included from ./pgscript/expressions/pgsCast.cpp:17:0:
pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
./pgscript/expressions/pgsCast.cpp: In member function ‘virtual
pgsOperand pgsCast::eval(pgsVarMap&) const’:
./pgscript/expressions/pgsCast.cpp:94:23: error: invalid use of
incomplete type ‘struct pgsRecord’
../pgadmin/include/pgscript/objects/pgsVariable.h:19:7: error: forward
declaration of ‘struct pgsRecord’
./pgscript/expressions/pgsCast.cpp:96:23: error: invalid use of
incomplete type ‘struct pgsString’
../pgadmin/include/pgscript/objects/pgsVariable.h:20:7: error: forward
declaration of ‘struct pgsString’
make[2]: [pgsCast.o] Error 1 (ignored)
mv -f .deps/pgsCast.Tpo .deps/pgsCast.Po
mv: cannot stat `.deps/pgsCast.Tpo': No such file or directory
make[2]: [pgsCast.o] Error 1 (ignored)
g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
pgsDriver.o -MD -MP -MF .deps/pgsDriver.Tpo -c -o pgsDriver.o `test -f
'./pgscript/utilities/pgsDriver.cpp' || echo
'./'`./pgscript/utilities/pgsDriver.cpp
In file included from ../pgadmin/include/pgscript/utilities/pgsScanner.h:33:0,
from ../pgadmin/include/pgscript/utilities/pgsDriver.h:15,
from ./pgscript/utilities/pgsDriver.cpp:12:
pgscript/pgsParser.yy:109:2: error: ‘pgsExpression’ does not name a type
pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
./pgscript/utilities/pgsDriver.cpp: In member function ‘bool
pgscript::pgsDriver::parse_stream(std::istream&)’:
./pgscript/utilities/pgsDriver.cpp:42:9: error: ‘class
pgscript::pgsParser’ has no member named ‘set_debug_level’
./pgscript/utilities/pgsDriver.cpp: In member function ‘void
pgscript::pgsDriver::error(const pgscript::location&, const
wxString&)’:
./pgscript/utilities/pgsDriver.cpp:75:9: error: invalid use of
incomplete type ‘struct pgsContext’
../pgadmin/include/pgscript/utilities/pgsDriver.h:19:7: error: forward
declaration of ‘struct pgsContext’
make[2]: [pgsDriver.o] Error 1 (ignored)
mv -f .deps/pgsDriver.Tpo .deps/pgsDriver.Po
mv: cannot stat `.deps/pgsDriver.Tpo': No such file or directory
make[2]: [pgsDriver.o] Error 1 (ignored)
g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
pgsThread.o -MD -MP -MF .deps/pgsThread.Tpo -c -o pgsThread.o `test -f
'./pgscript/utilities/pgsThread.cpp' || echo
'./'`./pgscript/utilities/pgsThread.cpp
mv -f .deps/pgsThread.Tpo .deps/pgsThread.Po

Here are the versions I've been using:

[peter(at)localhost pgscript]$ flex --version
flex 2.5.35
[peter(at)localhost pgscript]$ bison --version
bison (GNU Bison) 2.4.3
Written by Robert Corbett and Richard Stallman.

There is no obvious explanation for pgsExpression and the other
statements apparently not being defined - it's difficult to imagine a
new wx version having this effect. On the other hand, I don't know
much about lexx and yacc. Does anyone know what the problem might be?

When I'm done with this, I'll run the pgscript unit tests.

--
Regards,
Peter Geoghegan

Seems I omitted to copy my system's FlexLexer.h to
./pgadmin3/pgadmin/include/pgscript. Seemingly the FlexLexer.h that
ships with pgadmin is for Flex 2.5.33 only. I have 2.5.35.

However, I now get a slightly different set of compiler errors, so I'm
not much better off.

pgscript/lex.pgs.cc:944:10: error: ‘yy_buffer_stack’ was not declared
in this scope
pgscript/lex.pgs.cc:944:10: error: ‘yy_buffer_stack_top’ was not
declared in this scope
pgscript/lex.pgs.cc:945:27: error: ‘yyensure_buffer_stack’ was not
declared in this scope
pgscript/lex.pgs.cc:1858:8: error: ‘yy_buffer_stack’ was not declared
in this scope

...and so on...

Why do we ship a copy of FlexLexer.h from a specific version of
flex++? Are we really that sensitive to the differences in Flex
versions? Is the easiest thing to get pgadmin to build after changing
the pgscript grammar to use flex 2.5.33, as perhaps suggested in
./pgadmin3/pgadmin/pgscript/README?

--
Regards,
Peter Geoghegan

Sorry - I don't know the first thing about bison/flex. Magnus took
charge of that patch though - hopefully he knows more!

On Tue, Jan 18, 2011 at 8:29 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> Hmmm....I've come up against a problem.
>
> I found that when I ran the shell script parser.sh, bison and flex
> were run on the pgscript context-free grammar. My casts seem to work.
> However, a new problem has emerged. Here's part of the make output:
>
> g++ -DHAVE_CONFIG_H -I. -I..   -I/usr/local/pgsql/include
> -I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
> -I/usr/local/lib/wx/include/gtk2-unicode-2.9
> -I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
> -D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
> -I/usr/include/libxml2
> -DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
> -Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include   -MT
> lex.pgs.o -MD -MP -MF .deps/lex.pgs.Tpo -c -o lex.pgs.o `test -f
> './pgscript/lex.pgs.cc' || echo './'`./pgscript/lex.pgs.cc
> pgscript/lex.pgs.cc:315:25: error: no ‘int pgsFlexLexer::yywrap()’
> member function declared in class ‘pgsFlexLexer’
> In file included from pgscript/pgsScanner.ll:17:0:
> pgscript/pgsParser.yy:109:2: error: ‘pgsExpression’ does not name a type
> pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
> pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
> make[2]: [lex.pgs.o] Error 1 (ignored)
> mv -f .deps/lex.pgs.Tpo .deps/lex.pgs.Po
> mv: cannot stat `.deps/lex.pgs.Tpo': No such file or directory
> make[2]: [lex.pgs.o] Error 1 (ignored)
> g++ -DHAVE_CONFIG_H -I. -I..   -I/usr/local/pgsql/include
> -I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
> -I/usr/local/lib/wx/include/gtk2-unicode-2.9
> -I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
> -D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
> -I/usr/include/libxml2
> -DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
> -Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include   -MT
> parser.tab.o -MD -MP -MF .deps/parser.tab.Tpo -c -o parser.tab.o `test
> -f './pgscript/parser.tab.cc' || echo './'`./pgscript/parser.tab.cc
> mv -f .deps/parser.tab.Tpo .deps/parser.tab.Po
> g++ -DHAVE_CONFIG_H -I. -I..   -I/usr/local/pgsql/include
> -I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
> -I/usr/local/lib/wx/include/gtk2-unicode-2.9
> -I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
> -D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
> -I/usr/include/libxml2
> -DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
> -Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include   -MT
> pgsCast.o -MD -MP -MF .deps/pgsCast.Tpo -c -o pgsCast.o `test -f
> './pgscript/expressions/pgsCast.cpp' || echo
> './'`./pgscript/expressions/pgsCast.cpp
> In file included from ./pgscript/expressions/pgsCast.cpp:17:0:
> pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
> pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
> ./pgscript/expressions/pgsCast.cpp: In member function ‘virtual
> pgsOperand pgsCast::eval(pgsVarMap&) const’:
> ./pgscript/expressions/pgsCast.cpp:94:23: error: invalid use of
> incomplete type ‘struct pgsRecord’
> ../pgadmin/include/pgscript/objects/pgsVariable.h:19:7: error: forward
> declaration of ‘struct pgsRecord’
> ./pgscript/expressions/pgsCast.cpp:96:23: error: invalid use of
> incomplete type ‘struct pgsString’
> ../pgadmin/include/pgscript/objects/pgsVariable.h:20:7: error: forward
> declaration of ‘struct pgsString’
> make[2]: [pgsCast.o] Error 1 (ignored)
> mv -f .deps/pgsCast.Tpo .deps/pgsCast.Po
> mv: cannot stat `.deps/pgsCast.Tpo': No such file or directory
> make[2]: [pgsCast.o] Error 1 (ignored)
> g++ -DHAVE_CONFIG_H -I. -I..   -I/usr/local/pgsql/include
> -I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
> -I/usr/local/lib/wx/include/gtk2-unicode-2.9
> -I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
> -D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
> -I/usr/include/libxml2
> -DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
> -Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include   -MT
> pgsDriver.o -MD -MP -MF .deps/pgsDriver.Tpo -c -o pgsDriver.o `test -f
> './pgscript/utilities/pgsDriver.cpp' || echo
> './'`./pgscript/utilities/pgsDriver.cpp
> In file included from ../pgadmin/include/pgscript/utilities/pgsScanner.h:33:0,
>                 from ../pgadmin/include/pgscript/utilities/pgsDriver.h:15,
>                 from ./pgscript/utilities/pgsDriver.cpp:12:
> pgscript/pgsParser.yy:109:2: error: ‘pgsExpression’ does not name a type
> pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
> pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
> ./pgscript/utilities/pgsDriver.cpp: In member function ‘bool
> pgscript::pgsDriver::parse_stream(std::istream&)’:
> ./pgscript/utilities/pgsDriver.cpp:42:9: error: ‘class
> pgscript::pgsParser’ has no member named ‘set_debug_level’
> ./pgscript/utilities/pgsDriver.cpp: In member function ‘void
> pgscript::pgsDriver::error(const pgscript::location&, const
> wxString&)’:
> ./pgscript/utilities/pgsDriver.cpp:75:9: error: invalid use of
> incomplete type ‘struct pgsContext’
> ../pgadmin/include/pgscript/utilities/pgsDriver.h:19:7: error: forward
> declaration of ‘struct pgsContext’
> make[2]: [pgsDriver.o] Error 1 (ignored)
> mv -f .deps/pgsDriver.Tpo .deps/pgsDriver.Po
> mv: cannot stat `.deps/pgsDriver.Tpo': No such file or directory
> make[2]: [pgsDriver.o] Error 1 (ignored)
> g++ -DHAVE_CONFIG_H -I. -I..   -I/usr/local/pgsql/include
> -I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
> -I/usr/local/lib/wx/include/gtk2-unicode-2.9
> -I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
> -D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
> -I/usr/include/libxml2
> -DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
> -Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include   -MT
> pgsThread.o -MD -MP -MF .deps/pgsThread.Tpo -c -o pgsThread.o `test -f
> './pgscript/utilities/pgsThread.cpp' || echo
> './'`./pgscript/utilities/pgsThread.cpp
> mv -f .deps/pgsThread.Tpo .deps/pgsThread.Po
>
> Here are the versions I've been using:
>
> [peter(at)localhost pgscript]$ flex --version
> flex 2.5.35
> [peter(at)localhost pgscript]$ bison --version
> bison (GNU Bison) 2.4.3
> Written by Robert Corbett and Richard Stallman.
>
> There is no obvious explanation for pgsExpression and the other
> statements apparently not being defined - it's difficult to imagine a
> new wx version having this effect. On the other hand, I don't know
> much about lexx and yacc. Does anyone know what the problem might be?
>
> When I'm done with this, I'll run the pgscript unit tests.
>
> --
> Regards,
> Peter Geoghegan
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

And hopefully Magnus will be able to help here too!

On Tue, Jan 18, 2011 at 11:25 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> Seems I omitted to copy my system's FlexLexer.h to
> ./pgadmin3/pgadmin/include/pgscript. Seemingly the FlexLexer.h that
> ships with pgadmin is for Flex 2.5.33 only. I have 2.5.35.
>
> However, I now get a slightly different set of compiler errors, so I'm
> not much better off.
>
> pgscript/lex.pgs.cc:944:10: error: ‘yy_buffer_stack’ was not declared
> in this scope
> pgscript/lex.pgs.cc:944:10: error: ‘yy_buffer_stack_top’ was not
> declared in this scope
> pgscript/lex.pgs.cc:945:27: error: ‘yyensure_buffer_stack’ was not
> declared in this scope
> pgscript/lex.pgs.cc:1858:8: error: ‘yy_buffer_stack’ was not declared
> in this scope
>
> ...and so on...
>
> Why do we ship a copy of FlexLexer.h from a specific version of
> flex++? Are we really that sensitive to the differences in Flex
> versions? Is the easiest thing to get pgadmin to build after changing
> the pgscript grammar to use flex 2.5.33, as perhaps suggested in
> ./pgadmin3/pgadmin/pgscript/README?
>
> --
> Regards,
> Peter Geoghegan
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I'm going to try it with Flex 2.5.33. It stands to reason that
whatever the problem is, it is down to a problem in the original
grammar rather than the tiny changes I made to the embedded C++ code.

--
Regards,
Peter Geoghegan

On Wed, Jan 19, 2011 at 11:20 AM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> I'm going to try it with Flex 2.5.33. It stands to reason that
> whatever the problem is, it is down to a problem in the original
> grammar rather than the tiny changes I made to the embedded C++ code.

FYI, my laptop has:

raptor:~ dpage$ flex --version
flex 2.5.35

Just to confuse matters :-)

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> FYI, my laptop has:
>
> raptor:~ dpage$ flex --version
> flex 2.5.35
>
> Just to confuse matters :-)

Right, but I think that there isn't a dependency on flex...flex/bison
generated source code is committed to the tree, so there isn't a
requirement for people to run them themselves. In this regard, the
PgAdmin tree is similar to official releases of Postgres that are
shipped with that crud, but not the Postgres tree itself.

Looks like Magnus just committed a fix...

--
Regards,
Peter Geoghegan

On Jan 19, 2011 12:32 PM, "Peter Geoghegan" <peter(dot)geoghegan86(at)gmail(dot)com>
wrote:
>
> > FYI, my laptop has:
> >
> > raptor:~ dpage$ flex --version
> > flex 2.5.35
> >
> > Just to confuse matters :-)
>
> Right, but I think that there isn't a dependency on flex...flex/bison
> generated source code is committed to the tree, so there isn't a
> requirement for people to run them themselves. In this regard, the
> PgAdmin tree is similar to official releases of Postgres that are
> shipped with that crud, but not the Postgres tree itself.
>
> Looks like Magnus just committed a fix...
>

No that was another thing i had forgotten to push. I haven't time to look at
this one yet.

/Magnus

Using flex 2.5.33 hasn't helped :-(

--
Regards,
Peter Geoghegan

I decided to copy the OGL headers into /include/wx/ogl and proceed
with getting the remaining translation units to compile, where
compilation was completely aborted before due to ogl.h not being
available. I've posted to wx users, and to bakefile-devel, and will
hopefully have a solution to my linker problem soon. I've written my
own little wrapper function for Scintilla encoding functions that will
be temporarily used as a drop in replacement for wx2stc(). There is no
standard header that I can include for the Scintilla stuff (2
functions originally declared in UniConversion.h), so I've written
some extern declarations myself. I realise that this isn't acceptable
for production, but I think it's silly to worry about it until I can
actually run the code through a debugger (i.e. the eventual
implementation that uses Wx utilities only).

Now, all object files compiled, except where we have Bison/Flex problems.

I have attached a patch for your information. Obviously, I am not
asking you to commit it. Most warnings have been fixed - I didn't fix
one in gqbGridPanel::OnButtonUp(wxCommandEvent&) where I think we
downcast with a C style cast.

I think I found a bug in pgRole.cpp at line 304, where we compared a
long to a wxString. Obviously wxString's newfound stronger typing
highlighted this. Comparing strings of OIDs instead ought to be safe,
right?

Not quite sure how to proceed with bringing sqlGridBoolEditor (which
inherits from wxGridCellEditor) up to speed with 2.9, but that should
be clearer when I can debug. changes.txt says
"wxGridCellEditor::EndEdit() signature has changed and it was split in
two functions, one still called EndEdit() and ApplyEdit() (both are
pure, and ApplyEdit has no actual implementation). See the
documentation of the new functions for more details about how grid
editors should be written now". The docs say of AppyEdit()
"Effectively save the changes in the grid. This function should save
the value of the control in the grid. It is called only after
EndEdit() returns true."

--
Regards,
Peter Geoghegan

On Wed, Jan 19, 2011 at 6:04 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> I decided to copy the OGL headers into /include/wx/ogl and proceed
> with getting the remaining translation units to compile, where
> compilation was completely aborted before due to ogl.h not being
> available. I've posted to wx users, and to bakefile-devel, and will
> hopefully have a solution to my linker problem soon. I've written my
> own little wrapper function for Scintilla encoding functions that will
> be temporarily used as a drop in replacement for wx2stc(). There is no
> standard header that I can include for the Scintilla stuff (2
> functions originally declared in UniConversion.h), so I've written
> some extern declarations myself. I realise that this isn't acceptable
> for production, but I think it's silly to worry about it until I can
> actually run the code through a debugger  (i.e. the eventual
> implementation that uses Wx utilities only).

OK.

> Now, all object files compiled, except where we have Bison/Flex problems.

:-(

> I have attached a patch for your information. Obviously, I am not
> asking you to commit it. Most warnings have been fixed - I didn't fix
> one in gqbGridPanel::OnButtonUp(wxCommandEvent&) where I think we
> downcast with a C style cast.

Hmm, I didn't spot anything in there that looked horrendously
objectional. The "foo.c_str()" -> "(const char *) foo" changes are
somewhat ugly though

> I think I found a bug in pgRole.cpp at line 304, where we compared a
> long to a wxString. Obviously wxString's newfound stronger typing
> highlighted this. Comparing strings of OIDs instead ought to be safe,
> right?

Oops. Yep, that should be fine.

> Not quite sure how to proceed with bringing sqlGridBoolEditor (which
> inherits from wxGridCellEditor) up to speed with 2.9, but that should
> be clearer when I can debug. changes.txt says
> "wxGridCellEditor::EndEdit() signature has changed and it was split in
> two functions, one still called EndEdit() and ApplyEdit() (both are
> pure, and ApplyEdit has no actual implementation). See the
> documentation of the new functions for more details about how grid
> editors should be written now". The docs say of AppyEdit()
> "Effectively save the changes in the grid. This function should save
> the value of the control in the grid. It is called only after
> EndEdit() returns true."

I assume, just move:

grid->GetTable()->SetValue(row, col, value);

into ApplyEdit() ?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 19 January 2011 22:40, Dave Page <dpage(at)pgadmin(dot)org> wrote:

>> I have attached a patch for your information. Obviously, I am not
>> asking you to commit it. Most warnings have been fixed - I didn't fix
>> one in gqbGridPanel::OnButtonUp(wxCommandEvent&) where I think we
>> downcast with a C style cast.
>
> Hmm, I didn't spot anything in there that looked horrendously
> objectional. The "foo.c_str()" -> "(const char *) foo" changes are
> somewhat ugly though

I agree, but that's the best we can do while targeting both 2.8 and
2.9. There's a similar situation with switch statements having to be
cast to integer types - when we stop supporting 2.8, we can remove the
casts and call GetValue() on the proxy object.

--
Regards,
Peter Geoghegan

I took another look at getting OGL to build against 2.9 this evening
(using Julian Smart's new and improved version with the ogl.tar.gz
bakefile), and was successful. I hacked the Makefile to add -fPIC to
CXXFLAGS. I'm working on a less hacky solution (basically, fixing the
included Bakefile). I don't think that the included samples (which are
are supposed to be built as binary executables) should have that flag.
When I run the "studio" sample from the tree directly, I get this
error:

[peter(at)localhost studio]$ ./ogl2
./ogl2: error while loading shared libraries:
libwxcode_gtk2u_ogl-2.9.so.0: cannot open shared object file: No such
file or directory

This is despite the fact that the relevant .so appears to be available:

[peter(at)localhost lib]$ pwd
/usr/local/lib
[peter(at)localhost lib]$ ls -l *ogl*
lrwxrwxrwx. 1 root root 28 Jan 22 20:48 libwxcode_gtk2u_ogl-2.9.so
-> libwxcode_gtk2u_ogl-2.9.so.0
lrwxrwxrwx. 1 root root 32 Jan 22 20:48
libwxcode_gtk2u_ogl-2.9.so.0 -> libwxcode_gtk2u_ogl-2.9.so.0.0.0
-rwxr-xr-x. 1 root root 676304 Jan 22 20:48 libwxcode_gtk2u_ogl-2.9.so.0.0.0

I'm also concerned that the make install step doesn't install headers
anywhere. I guess that there's an expectation that the end-user should
manage that. It's not particularly clear what we should do about this.

--
Regards,
Peter Geoghegan

On Sat, Jan 22, 2011 at 9:08 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> I took another look at getting OGL to build against 2.9 this evening
> (using Julian Smart's new and improved version with the ogl.tar.gz
> bakefile), and was successful. I hacked the Makefile to add -fPIC to
> CXXFLAGS. I'm working on a less hacky solution (basically, fixing the
> included Bakefile). I don't think that the included samples (which are
> are supposed to be built as binary executables) should have that flag.
> When I run the "studio" sample from the tree directly, I get this
> error:
>
> [peter(at)localhost studio]$ ./ogl2
> ./ogl2: error while loading shared libraries:
> libwxcode_gtk2u_ogl-2.9.so.0: cannot open shared object file: No such
> file or directory
>
> This is despite the fact that the relevant .so appears to be available:
>
> [peter(at)localhost lib]$ pwd
> /usr/local/lib
> [peter(at)localhost lib]$ ls -l *ogl*
> lrwxrwxrwx. 1 root root     28 Jan 22 20:48 libwxcode_gtk2u_ogl-2.9.so
> -> libwxcode_gtk2u_ogl-2.9.so.0
> lrwxrwxrwx. 1 root root     32 Jan 22 20:48
> libwxcode_gtk2u_ogl-2.9.so.0 -> libwxcode_gtk2u_ogl-2.9.so.0.0.0
> -rwxr-xr-x. 1 root root 676304 Jan 22 20:48 libwxcode_gtk2u_ogl-2.9.so.0.0.0
>
> I'm also concerned that the make install step doesn't install headers
> anywhere. I guess that there's an expectation that the end-user should
> manage that. It's not particularly clear what we should do about this.

Hmm - I'm currently wondering if we should enumerate all the
committers to that part of the wx tree (but not the bakefile, as we
wouldn't use it), and see if we can get permission from the rest of
them to relicence the code.

That should make life much easier, as we could just add the code to
our source tree, and use our own build system.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 22 January 2011 21:25, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> Hmm - I'm currently wondering if we should enumerate all the
> committers to that part of the wx tree (but not the bakefile, as we
> wouldn't use it), and see if we can get permission from the rest of
> them to relicence the code.
>
> That should make life much easier, as we could just add the code to
> our source tree, and use our own build system.

+1.

Is this something that you've actually made some headway on?

--
Regards,
Peter Geoghegan

On Sat, Jan 22, 2011 at 9:31 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> On 22 January 2011 21:25, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> Hmm - I'm currently wondering if we should enumerate all the
>> committers to that part of the wx tree (but not the bakefile, as we
>> wouldn't use it), and see if we can get permission from the rest of
>> them to relicence the code.
>>
>> That should make life much easier, as we could just add the code to
>> our source tree, and use our own build system.
>
> +1.
>
> Is this something that you've actually made some headway on?

I had a brief look to see if it was clear there were more contributors
than just Julian, but that's it.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jan 19, 2011 at 00:25, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> Seems I omitted to copy my system's FlexLexer.h to
> ./pgadmin3/pgadmin/include/pgscript. Seemingly the FlexLexer.h that
> ships with pgadmin is for Flex 2.5.33 only. I have 2.5.35.
>
> However, I now get a slightly different set of compiler errors, so I'm
> not much better off.
>
> pgscript/lex.pgs.cc:944:10: error: ‘yy_buffer_stack’ was not declared
> in this scope
> pgscript/lex.pgs.cc:944:10: error: ‘yy_buffer_stack_top’ was not
> declared in this scope
> pgscript/lex.pgs.cc:945:27: error: ‘yyensure_buffer_stack’ was not
> declared in this scope
> pgscript/lex.pgs.cc:1858:8: error: ‘yy_buffer_stack’ was not declared
> in this scope
>
> ...and so on...
>
> Why do we ship a copy of FlexLexer.h from a specific version of
> flex++? Are we really that sensitive to the differences in Flex
> versions? Is the easiest thing to get pgadmin to build after changing
> the pgscript grammar to use flex 2.5.33, as perhaps suggested in
> ./pgadmin3/pgadmin/pgscript/README?

Isn't that part of the "ship the bison/flex output files" thing? We
ship lex.pgs.cc parser.tab.cc as well, and that's where the dependency
comes from. So we need to ship either both of them or none of them, I
think.

I don't recall exactly why it was, but the README is pretty specific
about the fact that it *is* sensitive. So the version used to generate
the .cc files should be the same as the one used for the .h....

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 23 January 2011 12:58, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> Why do we ship a copy of FlexLexer.h from a specific version of
>> flex++? Are we really that sensitive to the differences in Flex
>> versions? Is the easiest thing to get pgadmin to build after changing
>> the pgscript grammar to use flex 2.5.33, as perhaps suggested in
>> ./pgadmin3/pgadmin/pgscript/README?
>
> Isn't that part of the "ship the bison/flex output files" thing? We
> ship lex.pgs.cc parser.tab.cc as well, and that's where the dependency
> comes from. So we need to ship either both of them or none of them, I
> think.

Right, I see your point.

> I don't recall exactly why it was, but the README is pretty specific
> about the fact that it *is* sensitive. So the version used to generate
> the .cc files should be the same as the one used for the .h....

Actually, I would say that the README isn't very clear on that at all
- it just indicates that non 2.5.33 Flex will invalidate the shipped
FlexLexer.h. However, I suppose despite what you've said about
FlexLexer.h, the fact that we ship what is supposed to be a system
header makes whether or not we're sensitive to Flex version less
clear.

We ship a shell script, "parser.sh", that does it all for us. Example:

cd ./.. ... done
/home/peter/pgadmin3/pgadmin

+ Generating Bison output... done
+ Generating Flex output... done
+ Processing Flex output... done
+ Processing Bison output... done
+ Moving Bison header files...
+ mv pgscript/location.hh include/pgscript/location.hh
+ mv pgscript/parser.tab.hh include/pgscript/parser.tab.hh
+ mv pgscript/position.hh include/pgscript/position.hh
+ mv pgscript/stack.hh include/pgscript/stack.hh
... done

This updates both lex.pgs.cc and parser.tab.cc . I now run Make
--ignore, and see errors about various structs only having
declarations and not definitions available. Here it is:

g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
lex.pgs.o -MD -MP -MF .deps/lex.pgs.Tpo -c -o lex.pgs.o `test -f
'./pgscript/lex.pgs.cc' || echo './'`./pgscript/lex.pgs.cc
In file included from pgscript/pgsScanner.ll:17:0:
pgscript/pgsParser.yy:109:2: error: ‘pgsExpression’ does not name a type
pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
make[2]: [lex.pgs.o] Error 1 (ignored)
mv -f .deps/lex.pgs.Tpo .deps/lex.pgs.Po
mv: cannot stat `.deps/lex.pgs.Tpo': No such file or directory
make[2]: [lex.pgs.o] Error 1 (ignored)
g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
pgsCast.o -MD -MP -MF .deps/pgsCast.Tpo -c -o pgsCast.o `test -f
'./pgscript/expressions/pgsCast.cpp' || echo
'./'`./pgscript/expressions/pgsCast.cpp
In file included from ./pgscript/expressions/pgsCast.cpp:17:0:
pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
./pgscript/expressions/pgsCast.cpp: In member function ‘virtual
pgsOperand pgsCast::eval(pgsVarMap&) const’:
./pgscript/expressions/pgsCast.cpp:94:23: error: invalid use of
incomplete type ‘struct pgsRecord’
../pgadmin/include/pgscript/objects/pgsVariable.h:19:7: error: forward
declaration of ‘struct pgsRecord’
./pgscript/expressions/pgsCast.cpp:96:23: error: invalid use of
incomplete type ‘struct pgsString’
../pgadmin/include/pgscript/objects/pgsVariable.h:20:7: error: forward
declaration of ‘struct pgsString’
make[2]: [pgsCast.o] Error 1 (ignored)
mv -f .deps/pgsCast.Tpo .deps/pgsCast.Po
mv: cannot stat `.deps/pgsCast.Tpo': No such file or directory
make[2]: [pgsCast.o] Error 1 (ignored)
g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
pgsDriver.o -MD -MP -MF .deps/pgsDriver.Tpo -c -o pgsDriver.o `test -f
'./pgscript/utilities/pgsDriver.cpp' || echo
'./'`./pgscript/utilities/pgsDriver.cpp
In file included from ../pgadmin/include/pgscript/utilities/pgsScanner.h:33:0,
from ../pgadmin/include/pgscript/utilities/pgsDriver.h:15,
from ./pgscript/utilities/pgsDriver.cpp:12:
pgscript/pgsParser.yy:109:2: error: ‘pgsExpression’ does not name a type
pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
./pgscript/utilities/pgsDriver.cpp: In member function ‘bool
pgscript::pgsDriver::parse_stream(std::istream&)’:
./pgscript/utilities/pgsDriver.cpp:42:9: error: ‘class
pgscript::pgsParser’ has no member named ‘set_debug_level’
./pgscript/utilities/pgsDriver.cpp: In member function ‘void
pgscript::pgsDriver::error(const pgscript::location&, const
wxString&)’:
./pgscript/utilities/pgsDriver.cpp:75:9: error: invalid use of
incomplete type ‘struct pgsContext’
../pgadmin/include/pgscript/utilities/pgsDriver.h:19:7: error: forward
declaration of ‘struct pgsContext’
make[2]: [pgsDriver.o] Error 1 (ignored)
mv -f .deps/pgsDriver.Tpo .deps/pgsDriver.Po
mv: cannot stat `.deps/pgsDriver.Tpo': No such file or directory
make[2]: [pgsDriver.o] Error 1 (ignored)

It's as various #includes are absent from various CPP files. I hack
the files listed in those various cpp files and add includes (mostly
the ones in pgsParset.yy, but some others - haven't bothered to
isolate anything), and everything seems to work fine.

Now, the only remaining error message is that prevents us from
compiling the only missing object file, pgsDriver.o, is:
./pgscript/utilities/pgsDriver.cpp: In member function ‘bool
pgscript::pgsDriver::parse_stream(std::istream&)’:
./pgscript/utilities/pgsDriver.cpp:49:9: error: ‘class
pgscript::pgsParser’ has no member named ‘set_debug_level’

The class pgsParser very clearly does not have a member named
set_debug_level, so it's hard to argue with that.

How would everyone feel about committing my work without Flex/Bison
changes, and without worrying about how we'll eventually handle OGL?
I'll verify that I haven't somehow broken wx 2.8 compatibility today,
and then produce a new patch.

--
Regards,
Peter Geoghegan

On Sun, Jan 23, 2011 at 15:46, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> On 23 January 2011 12:58, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> Why do we ship a copy of FlexLexer.h from a specific version of
>>> flex++? Are we really that sensitive to the differences in Flex
>>> versions? Is the easiest thing to get pgadmin to build after changing
>>> the pgscript grammar to use flex 2.5.33, as perhaps suggested in
>>> ./pgadmin3/pgadmin/pgscript/README?
>>
>> Isn't that part of the "ship the bison/flex output files" thing? We
>> ship lex.pgs.cc parser.tab.cc as well, and that's where the dependency
>> comes from. So we need to ship either both of them or none of them, I
>> think.
>
> Right, I see your point.
>
>> I don't recall exactly why it was, but the README is pretty specific
>> about the fact that it *is* sensitive. So the version used to generate
>> the .cc files should be the same as the one used for the .h....
>
> Actually, I would say that the README isn't very clear on that at all
> - it just indicates that non 2.5.33 Flex will invalidate the shipped
> FlexLexer.h. However, I suppose despite what you've said about
> FlexLexer.h, the fact that we ship what is supposed to be a system
> header makes whether or not we're sensitive to Flex version less
> clear.
>
> We ship a shell script, "parser.sh", that does it all for us. Example:
>
> cd ./.. ... done
> /home/peter/pgadmin3/pgadmin
>
> + Generating Bison output... done
> + Generating Flex output... done
> + Processing Flex output... done
> + Processing Bison output... done
> + Moving Bison header files...
>    + mv pgscript/location.hh include/pgscript/location.hh
>    + mv pgscript/parser.tab.hh include/pgscript/parser.tab.hh
>    + mv pgscript/position.hh include/pgscript/position.hh
>    + mv pgscript/stack.hh include/pgscript/stack.hh
> ... done

Hmm. Perhaps the bug is that parser.sh should also update FlexLexer.h?

> This updates both  lex.pgs.cc and parser.tab.cc . I now run Make
> --ignore, and see errors about various structs only having
> declarations and not definitions available. Here it is:
>
> g++ -DHAVE_CONFIG_H -I. -I..   -I/usr/local/pgsql/include
> -I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
> -I/usr/local/lib/wx/include/gtk2-unicode-2.9
> -I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
> -D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
> -I/usr/include/libxml2
> -DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
> -Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include   -MT
> lex.pgs.o -MD -MP -MF .deps/lex.pgs.Tpo -c -o lex.pgs.o `test -f
> './pgscript/lex.pgs.cc' || echo './'`./pgscript/lex.pgs.cc
> In file included from pgscript/pgsScanner.ll:17:0:
> pgscript/pgsParser.yy:109:2: error: ‘pgsExpression’ does not name a type
> pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
> pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type

> It's as various #includes are absent from various CPP files. I hack
> the files listed in those various cpp files and add includes (mostly
> the ones in pgsParset.yy, but some others - haven't bothered to
> isolate anything), and everything seems to work fine.

Yeah, certainly looks like missing include file(s) somewhere? Thought
it's weird this has stopped working, since it does work in branch head
- have you changed some global headers? (there are too many things in
general that are included in global headers, imho, which may hide
missing local ones i nsome cases)

(sorry, I've not been following this thread, just got alerteed to it
when dave pointed towards me)

> Now, the only remaining error message is that prevents us from
> compiling the only missing object file, pgsDriver.o, is:
> ./pgscript/utilities/pgsDriver.cpp: In member function ‘bool
> pgscript::pgsDriver::parse_stream(std::istream&)’:
> ./pgscript/utilities/pgsDriver.cpp:49:9: error: ‘class
> pgscript::pgsParser’ has no member named ‘set_debug_level’
>
> The class pgsParser very clearly does not have a member named
> set_debug_level, so it's hard to argue with that.

Well, in git head, it's in parser.tab.cc. So it seems bison generated
it then, and that has changed. The one generated is by Bison 2.3, do
you have a different verison of that as well? Perhaps it's documented
in the release notes for that one how this has changed?

> How would everyone feel about committing my work without Flex/Bison
> changes, and without worrying about how we'll eventually handle OGL?
> I'll verify that I haven't somehow broken wx 2.8 compatibility today,
> and then produce a new patch.

In principle I'm for incremental commits. Assuming the patch itself is
ok of course, I haven't actually reviewed anything :-)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 23 January 2011 14:56, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> Hmm. Perhaps the bug is that parser.sh should also update FlexLexer.h?

Hacking a system header and then shipping it without mentioning
anything seems like a bad idea, an idea that would be unlikely to fly.

> Yeah, certainly looks like missing include file(s) somewhere? Thought
> it's weird this has stopped working, since it does work in branch head
> - have you changed some global headers? (there are too many things in
> general that are included in global headers, imho, which may hide
> missing local ones i nsome cases)

Haven't removed any #includes at any point. The following files are
changed from master:

pgadmin/agent/pgaJob.cpp
pgadmin/ctl/calbox.cpp
pgadmin/ctl/ctlListView.cpp
pgadmin/ctl/ctlSQLBox.cpp
pgadmin/ctl/explainShape.cpp
pgadmin/ctl/timespin.cpp
pgadmin/db/pgConn.cpp
pgadmin/db/pgQueryThread.cpp
pgadmin/debugger/dbgPgConn.cpp
pgadmin/dlg/dlgExtTable.cpp
pgadmin/dlg/dlgSelectConnection.cpp
pgadmin/dlg/dlgView.cpp
pgadmin/frm/events.cpp
pgadmin/frm/frmEditGrid.cpp
pgadmin/frm/frmHbaConfig.cpp
pgadmin/frm/frmHint.cpp
pgadmin/frm/frmMainConfig.cpp
pgadmin/frm/frmPgpassConfig.cpp
pgadmin/frm/frmQuery.cpp
pgadmin/frm/frmReport.cpp
pgadmin/frm/frmStatus.cpp
pgadmin/gqb/gqbGraphSimple.cpp
pgadmin/gqb/gqbView.cpp
pgadmin/include/ctl/calbox.h
pgadmin/include/ctl/ctlListView.h
pgadmin/include/ctl/ctlSQLBox.h
pgadmin/include/pgscript/location.hh
pgadmin/include/pgscript/parser.tab.hh
pgadmin/include/pgscript/position.hh
pgadmin/include/pgscript/stack.hh
pgadmin/include/pgscript/statements/pgsStmt.h
pgadmin/include/utils/sysLogger.h
pgadmin/include/utils/sysSettings.h
pgadmin/pgAdmin3.cpp
pgadmin/pgscript/expressions/pgsCast.cpp
pgadmin/pgscript/generators/pgsDictionaryGen.cpp
pgadmin/pgscript/generators/pgsReferenceGen.cpp
pgadmin/pgscript/generators/pgsRegexGen.cpp
pgadmin/pgscript/lex.pgs.cc
pgadmin/pgscript/parser.sh
pgadmin/pgscript/parser.tab.cc
pgadmin/pgscript/pgsApplication.cpp
pgadmin/pgscript/pgsParser.yy
pgadmin/pgscript/utilities/pgsContext.cpp
pgadmin/pgscript/utilities/pgsDriver.cpp
pgadmin/schema/pgIndex.cpp
pgadmin/schema/pgObject.cpp
pgadmin/schema/pgRole.cpp
pgadmin/slony/dlgRepCluster.cpp
pgadmin/slony/slSet.cpp
pgadmin/utils/csvfiles.cpp
pgadmin/utils/factory.cpp
pgadmin/utils/pgconfig.cpp

>> The class pgsParser very clearly does not have a member named
>> set_debug_level, so it's hard to argue with that.
>
> Well, in git head, it's in parser.tab.cc. So it seems bison generated
> it then, and that has changed. The one generated is by Bison 2.3, do
> you have a different verison of that as well? Perhaps it's documented
> in the release notes for that one how this has changed?

I have Bison 2.4.3. Having taken a closer look, I see both the
declaration and definition of that function are in parser.tab.cc and
parset.tab.hh respectively. The declaration is within a #if YYDEBUG
block, so that would account for why the compiler doesn't see it. This
is a debug build, so I wonder why, though we seem to expect the
function to be there in either debug or release builds.

> In principle I'm for incremental commits. Assuming the patch itself is
> ok of course, I haven't actually reviewed anything :-)

Okay, good.

By the way, I'm seeing lots of warnings like this:

./dlg/dlgSequence.cpp:114:6: warning: suggest explicit braces to avoid
ambiguous ‘else’

Worth fixing?

--
Regards,
Peter Geoghegan

Le 23/01/2011 17:01, Peter Geoghegan a écrit :
> On 23 January 2011 14:56, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>
>> Hmm. Perhaps the bug is that parser.sh should also update FlexLexer.h?
>
> Hacking a system header and then shipping it without mentioning
> anything seems like a bad idea, an idea that would be unlikely to fly.
>
>> Yeah, certainly looks like missing include file(s) somewhere? Thought
>> it's weird this has stopped working, since it does work in branch head
>> - have you changed some global headers? (there are too many things in
>> general that are included in global headers, imho, which may hide
>> missing local ones i nsome cases)
>
> Haven't removed any #includes at any point. The following files are
> changed from master:
>
> pgadmin/agent/pgaJob.cpp
> pgadmin/ctl/calbox.cpp
> pgadmin/ctl/ctlListView.cpp
> pgadmin/ctl/ctlSQLBox.cpp
> pgadmin/ctl/explainShape.cpp
> pgadmin/ctl/timespin.cpp
> pgadmin/db/pgConn.cpp
> pgadmin/db/pgQueryThread.cpp
> pgadmin/debugger/dbgPgConn.cpp
> pgadmin/dlg/dlgExtTable.cpp
> pgadmin/dlg/dlgSelectConnection.cpp
> pgadmin/dlg/dlgView.cpp
> pgadmin/frm/events.cpp
> pgadmin/frm/frmEditGrid.cpp
> pgadmin/frm/frmHbaConfig.cpp
> pgadmin/frm/frmHint.cpp
> pgadmin/frm/frmMainConfig.cpp
> pgadmin/frm/frmPgpassConfig.cpp
> pgadmin/frm/frmQuery.cpp
> pgadmin/frm/frmReport.cpp
> pgadmin/frm/frmStatus.cpp
> pgadmin/gqb/gqbGraphSimple.cpp
> pgadmin/gqb/gqbView.cpp
> pgadmin/include/ctl/calbox.h
> pgadmin/include/ctl/ctlListView.h
> pgadmin/include/ctl/ctlSQLBox.h
> pgadmin/include/pgscript/location.hh
> pgadmin/include/pgscript/parser.tab.hh
> pgadmin/include/pgscript/position.hh
> pgadmin/include/pgscript/stack.hh
> pgadmin/include/pgscript/statements/pgsStmt.h
> pgadmin/include/utils/sysLogger.h
> pgadmin/include/utils/sysSettings.h
> pgadmin/pgAdmin3.cpp
> pgadmin/pgscript/expressions/pgsCast.cpp
> pgadmin/pgscript/generators/pgsDictionaryGen.cpp
> pgadmin/pgscript/generators/pgsReferenceGen.cpp
> pgadmin/pgscript/generators/pgsRegexGen.cpp
> pgadmin/pgscript/lex.pgs.cc
> pgadmin/pgscript/parser.sh
> pgadmin/pgscript/parser.tab.cc
> pgadmin/pgscript/pgsApplication.cpp
> pgadmin/pgscript/pgsParser.yy
> pgadmin/pgscript/utilities/pgsContext.cpp
> pgadmin/pgscript/utilities/pgsDriver.cpp
> pgadmin/schema/pgIndex.cpp
> pgadmin/schema/pgObject.cpp
> pgadmin/schema/pgRole.cpp
> pgadmin/slony/dlgRepCluster.cpp
> pgadmin/slony/slSet.cpp
> pgadmin/utils/csvfiles.cpp
> pgadmin/utils/factory.cpp
> pgadmin/utils/pgconfig.cpp
>
>>> The class pgsParser very clearly does not have a member named
>>> set_debug_level, so it's hard to argue with that.
>>
>> Well, in git head, it's in parser.tab.cc. So it seems bison generated
>> it then, and that has changed. The one generated is by Bison 2.3, do
>> you have a different verison of that as well? Perhaps it's documented
>> in the release notes for that one how this has changed?
>
> I have Bison 2.4.3. Having taken a closer look, I see both the
> declaration and definition of that function are in parser.tab.cc and
> parset.tab.hh respectively. The declaration is within a #if YYDEBUG
> block, so that would account for why the compiler doesn't see it. This
> is a debug build, so I wonder why, though we seem to expect the
> function to be there in either debug or release builds.
>
>> In principle I'm for incremental commits. Assuming the patch itself is
>> ok of course, I haven't actually reviewed anything :-)
>
> Okay, good.
>

Not sure I agree there. This is a huge list of files to be fixed to
have... nothing more. I'm also all for incremental commits as long as
they do something.

> By the way, I'm seeing lots of warnings like this:
>
> ./dlg/dlgSequence.cpp:114:6: warning: suggest explicit braces to avoid
> ambiguous ‘else’
>
> Worth fixing?
>

Every warning is worth fixing.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Sun, Jan 23, 2011 at 4:01 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
>
>> In principle I'm for incremental commits. Assuming the patch itself is
>> ok of course, I haven't actually reviewed anything :-)
>
> Okay, good.

Agreed. We're going to have to do this sooner or later, so let's make
it incremental and as easy to deal with as possible.

> By the way, I'm seeing lots of warnings like this:
>
> ./dlg/dlgSequence.cpp:114:6: warning: suggest explicit braces to avoid
> ambiguous ‘else’
>
> Worth fixing?

Yes, always - though I don't actually see a problem there in this
case. It looks pretty unambiguous to me (114 being the first line of
the else block):

// Find, and disable the USAGE ACL option if we're on pre 8.2
// 8.2+ only supports SELECT, UPDATE and USAGE
if (!connection->BackendMinimumVersion(8, 2))
{
// Disable the checkbox
if (!DisablePrivilege(wxT("USAGE")))
wxLogError(_("Failed to disable the USAGE
privilege checkbox!"));
}
else
{
if (!DisablePrivilege(wxT("INSERT")))
wxLogError(_("Failed to disable the INSERT
privilege checkbox!"));
if (!DisablePrivilege(wxT("DELETE")))
wxLogError(_("Failed to disable the DELETE
privilege checkbox!"));
if (!DisablePrivilege(wxT("RULE")))
wxLogError(_("Failed to disable the RULE
privilege checkbox!"));
if (!DisablePrivilege(wxT("REFERENCES")))
wxLogError(_("Failed to disable the REFERENCES
privilege checkbox!"));
if (!DisablePrivilege(wxT("TRIGGER")))
wxLogError(_("Failed to disable the TRIGGER
privilege checkbox!"));
}

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Jan 23, 2011 at 10:21 PM, Guillaume Lelarge
<guillaume(at)lelarge(dot)info> wrote:
>
> Not sure I agree there. This is a huge list of files to be fixed to
> have... nothing more. I'm also all for incremental commits as long as
> they do something.

Where "nothing more" == "wxWidgets 2.9 compatibility"?

That seems like a WIN to me.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Le 24/01/2011 09:43, Dave Page a écrit :
> On Sun, Jan 23, 2011 at 10:21 PM, Guillaume Lelarge
> <guillaume(at)lelarge(dot)info> wrote:
>>
>> Not sure I agree there. This is a huge list of files to be fixed to
>> have... nothing more. I'm also all for incremental commits as long as
>> they do something.
>
> Where "nothing more" == "wxWidgets 2.9 compatibility"?
>

That's not what I understood from Peter's email because his patch
doesn't take care of OGL, which means we would have a somewhat broken
pgAdmin (no GQB, no graphical EXPLAIN).

> That seems like a WIN to me.
>

Complete 2.9 compatibility would be a win. And I confess I fail to see
why we should commit this patch. What is the added value for this patch?

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Mon, Jan 24, 2011 at 9:38 AM, Guillaume Lelarge
<guillaume(at)lelarge(dot)info> wrote:
> Le 24/01/2011 09:43, Dave Page a écrit :
>> On Sun, Jan 23, 2011 at 10:21 PM, Guillaume Lelarge
>> <guillaume(at)lelarge(dot)info> wrote:
>>>
>>> Not sure I agree there. This is a huge list of files to be fixed to
>>> have... nothing more. I'm also all for incremental commits as long as
>>> they do something.
>>
>> Where "nothing more" == "wxWidgets 2.9 compatibility"?
>>
>
> That's not what I understood from Peter's email because his patch
> doesn't take care of OGL, which means we would have a somewhat broken
> pgAdmin (no GQB, no graphical EXPLAIN).
>
>> That seems like a WIN to me.
>>
>
> Complete 2.9 compatibility would be a win. And I confess I fail to see
> why we should commit this patch. What is the added value for this patch?

It is an *incremental* change on the way to 2.9 compatibility. Adding
patches in an incremental fashion like this is quite normal (it's been
done before on a number of occasions in PostgreSQL and pgAdmin) as it
makes them much easier to review and commit, than huge monolithic
patches. An incremental patch may not bring full compatibility, but it
addresses a specific set of issues, and does so in a way that doesn't
break the existing builds.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Le 24/01/2011 10:49, Dave Page a écrit :
> On Mon, Jan 24, 2011 at 9:38 AM, Guillaume Lelarge
> <guillaume(at)lelarge(dot)info> wrote:
>> Le 24/01/2011 09:43, Dave Page a écrit :
>>> On Sun, Jan 23, 2011 at 10:21 PM, Guillaume Lelarge
>>> <guillaume(at)lelarge(dot)info> wrote:
>>>>
>>>> Not sure I agree there. This is a huge list of files to be fixed to
>>>> have... nothing more. I'm also all for incremental commits as long as
>>>> they do something.
>>>
>>> Where "nothing more" == "wxWidgets 2.9 compatibility"?
>>>
>>
>> That's not what I understood from Peter's email because his patch
>> doesn't take care of OGL, which means we would have a somewhat broken
>> pgAdmin (no GQB, no graphical EXPLAIN).
>>
>>> That seems like a WIN to me.
>>>
>>
>> Complete 2.9 compatibility would be a win. And I confess I fail to see
>> why we should commit this patch. What is the added value for this patch?
>
> It is an *incremental* change on the way to 2.9 compatibility. Adding
> patches in an incremental fashion like this is quite normal (it's been
> done before on a number of occasions in PostgreSQL and pgAdmin) as it
> makes them much easier to review and commit, than huge monolithic
> patches. An incremental patch may not bring full compatibility, but it
> addresses a specific set of issues, and does so in a way that doesn't
> break the existing builds.
>

Fair enough.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

I'd like to run the unit tests and regression tests
(/pgadmin3/xtra/pgscript/test), against our hacked grammar (that is,
our newly generated CPP files that have the additional includes). I
seem to be having much the same problems building them as PgAdmin
itself.

In file included from ./pgsTestExpressionCast.cpp:16:0:
pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
./pgsTestExpressionCast.cpp: In member function ‘void
pgsTestSuite::test_expression_cast()’:
./pgsTestExpressionCast.cpp:21:2: error: ‘pgsCast’ was not declared in
this scope
./pgsTestExpressionCast.cpp:21:12: error: ‘cast’ was not declared in this scope
./pgsTestExpressionCast.cpp:29:43: error: invalid use of incomplete
type ‘struct pgsNumber’
** SNIP **

I performed my usual hack, and get includes from pgsParser.yy:

#include "pgscript/pgScript.h" // not actually needed
#include "pgscript/statements/pgsStatements.h" // not actually needed
#include "pgscript/expressions/pgsExpressions.h" // needed
#include "pgscript/objects/pgsObjects.h" // needed
#include "pgscript/utilities/pgsContext.h" // needed

and the TU compiles.

I now get a linker error: make: *** No rule to make target
`../lib/libpgs.a', needed by `pgsTest'. Stop.

What's libpgs? Presumably it's the PgScript library, but it doesn't
appear to be available from anywhere.

Should I give up on doing anything with the grammar for now? My
inclination is to try and tackle the problem, and just don't address
OGL issues in this initial patch.

--
Regards,
Peter Geoghegan

On Tue, Jan 25, 2011 at 3:10 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> I'd like to run the unit tests and regression tests
> (/pgadmin3/xtra/pgscript/test), against our hacked grammar (that is,
> our newly generated CPP files that have the additional includes). I
> seem to be having much the same problems building them as PgAdmin
> itself.
>
> In file included from ./pgsTestExpressionCast.cpp:16:0:
> pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
> pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
> ./pgsTestExpressionCast.cpp: In member function ‘void
> pgsTestSuite::test_expression_cast()’:
> ./pgsTestExpressionCast.cpp:21:2: error: ‘pgsCast’ was not declared in
> this scope
> ./pgsTestExpressionCast.cpp:21:12: error: ‘cast’ was not declared in this scope
> ./pgsTestExpressionCast.cpp:29:43: error: invalid use of incomplete
> type ‘struct pgsNumber’
> ** SNIP **
>
> I performed my usual hack, and get includes from pgsParser.yy:
>
> #include "pgscript/pgScript.h" // not actually needed
> #include "pgscript/statements/pgsStatements.h" // not actually needed
> #include "pgscript/expressions/pgsExpressions.h" // needed
> #include "pgscript/objects/pgsObjects.h" // needed
> #include "pgscript/utilities/pgsContext.h" // needed
>
> and the TU compiles.
>
> I now get a linker error: make: *** No rule to make target
> `../lib/libpgs.a', needed by `pgsTest'.  Stop.
>
> What's libpgs? Presumably it's the PgScript library, but it doesn't
> appear to be available from anywhere.
>
> Should I give up on doing anything with the grammar for now? My
> inclination is to try and tackle the problem, and just don't address
> OGL issues in this initial patch.

Mr Hagander - that be your code :-p. Do you recall what/where that
library comes from?

Peter - xtra/pgscript really is unmaintained. Iirc, we only put the
code there so it had a home (it was already written when we added
pgScript).

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I had another look at ctlSQLBox::RegexFindText() this evening. I'm
just going to use the equivalent wx functionality. So, our drop in
replacement for wx2stc() is:

wxWX2MBbuf ctlSQLBox::StrStc(const wxString& str)
{

#if wxUSE_UNICODE
return str.mb_str(wxConvUTF8);
#else
return str.mbc_str();
#endif

}

This appears to work fine. Objections?

--
Regards,
Peter Geoghegan

On Tue, Jan 25, 2011 at 11:56 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> I had another look at ctlSQLBox::RegexFindText() this evening. I'm
> just going to use the equivalent wx functionality. So, our drop in
> replacement for wx2stc() is:
>
> wxWX2MBbuf ctlSQLBox::StrStc(const wxString& str)
> {
>
> #if wxUSE_UNICODE
>        return str.mb_str(wxConvUTF8);
> #else
>        return str.mbc_str();
> #endif
>
> }
>
> This appears to work fine. Objections?

We don't support non-unicode builds any more, so you might as well
just inline the mb_str call where it's needed, and lose the rest.

Otherwise, no objections :-p

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 25, 2011 at 16:29, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> On Tue, Jan 25, 2011 at 3:10 PM, Peter Geoghegan
> <peter(dot)geoghegan86(at)gmail(dot)com> wrote:
>> I'd like to run the unit tests and regression tests
>> (/pgadmin3/xtra/pgscript/test), against our hacked grammar (that is,
>> our newly generated CPP files that have the additional includes). I
>> seem to be having much the same problems building them as PgAdmin
>> itself.
>>
>> In file included from ./pgsTestExpressionCast.cpp:16:0:
>> pgscript/pgsParser.yy:110:2: error: ‘pgsStmt’ does not name a type
>> pgscript/pgsParser.yy:111:2: error: ‘pgsStmtList’ does not name a type
>> ./pgsTestExpressionCast.cpp: In member function ‘void
>> pgsTestSuite::test_expression_cast()’:
>> ./pgsTestExpressionCast.cpp:21:2: error: ‘pgsCast’ was not declared in
>> this scope
>> ./pgsTestExpressionCast.cpp:21:12: error: ‘cast’ was not declared in this scope
>> ./pgsTestExpressionCast.cpp:29:43: error: invalid use of incomplete
>> type ‘struct pgsNumber’
>> ** SNIP **
>>
>> I performed my usual hack, and get includes from pgsParser.yy:
>>
>> #include "pgscript/pgScript.h" // not actually needed
>> #include "pgscript/statements/pgsStatements.h" // not actually needed
>> #include "pgscript/expressions/pgsExpressions.h" // needed
>> #include "pgscript/objects/pgsObjects.h" // needed
>> #include "pgscript/utilities/pgsContext.h" // needed
>>
>> and the TU compiles.
>>
>> I now get a linker error: make: *** No rule to make target
>> `../lib/libpgs.a', needed by `pgsTest'.  Stop.
>>
>> What's libpgs? Presumably it's the PgScript library, but it doesn't
>> appear to be available from anywhere.
>>
>> Should I give up on doing anything with the grammar for now? My
>> inclination is to try and tackle the problem, and just don't address
>> OGL issues in this initial patch.
>
> Mr Hagander - that be your code :-p. Do you recall what/where that
> library comes from?

It is? ;)

libpgs is pgscript, yes. It's built in xtra/pgscript/lib.

> Peter - xtra/pgscript really is unmaintained. Iirc, we only put the
> code there so it had a home (it was already written when we added
> pgScript).

Well, I applied a patch a couple of days back that made it build again.

However, I never actually ran the tests. So I'd start by verifying if
that works on an *unpatched* system, really :-)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 26 January 2011 09:15, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> We don't support non-unicode builds any more, so you might as well
> just inline the mb_str call where it's needed, and lose the rest.

Okay, I removed the entire function.

I had another look at our Flex/Bison problem. Look at this diff
output, after a fresh run of parser.sh:

diff --git a/pgadmin/include/pgscript/parser.tab.hh
b/pgadmin/include/pgscript/parser.tab.hh
--- a/pgadmin/include/pgscript/parser.tab.hh
+++ b/pgadmin/include/pgscript/parser.tab.hh
#include <string>
#include <iostream>
#include "stack.hh"
-#include "pgscript/pgScript.h"
-#include "pgscript/statements/pgsStatements.h"
-#include "pgscript/expressions/pgsExpressions.h"
-#include "pgscript/objects/pgsObjects.h"
-#include "pgscript/utilities/pgsContext.h"
#include "location.hh"

Restoring this by hacking the header doesn't quite fix the problem.
More headers must be hacked, where there doesn't appear to have been
headers removed. This may be symptomatic of a particular problem that
someone that really groks Flex++/Bison may be able to identify. What
do you think?

Regarding warnings, some cross-platform projects standardise on fixing
all warnings for a particular preferred platform. I thought you might
not appreciate fixes for warnings that you presumably don't even see
on your system (otherwise you'd have fixed them yourself), that crop
up quite a lot, on top of an already fairly extensive patch. I'll fix
them though.

BTW, pgscript developer docs indicate that Flex 2.5.33+ is supported,
so it obviously isn't that sensitive.

--
Regards,
Peter Geoghegan

On Wed, Jan 26, 2011 at 12:56 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> On 26 January 2011 09:15, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> We don't support non-unicode builds any more, so you might as well
>> just inline the mb_str call where it's needed, and lose the rest.
>
> Okay, I removed the entire function.

*nod*

> I had another look at our Flex/Bison problem. Look at this diff
> output, after a fresh run of parser.sh:
>
> diff --git a/pgadmin/include/pgscript/parser.tab.hh
> b/pgadmin/include/pgscript/parser.tab.hh
> --- a/pgadmin/include/pgscript/parser.tab.hh
> +++ b/pgadmin/include/pgscript/parser.tab.hh
>  #include <string>
>  #include <iostream>
>  #include "stack.hh"
> -#include "pgscript/pgScript.h"
> -#include "pgscript/statements/pgsStatements.h"
> -#include "pgscript/expressions/pgsExpressions.h"
> -#include "pgscript/objects/pgsObjects.h"
> -#include "pgscript/utilities/pgsContext.h"
>  #include "location.hh"
>
> Restoring this by hacking the header doesn't quite fix the problem.
> More headers must be hacked, where there doesn't appear to have been
> headers removed. This may be symptomatic of a particular problem that
> someone that really groks Flex++/Bison may be able to identify. What
> do you think?

Seems like a reasonable assumption - but I'm not that flex/bison
person unfortunately. Is there a mailing list for those tools?

> Regarding warnings, some cross-platform projects standardise on fixing
> all warnings for a particular preferred platform. I thought you might
> not appreciate fixes for warnings that you presumably don't even see
> on your system (otherwise you'd have fixed them yourself), that crop
> up quite a lot, on top of an already fairly extensive patch. I'll fix
> them though.

Personally, I expect zero warnings on any platform I work on. I
currently test on OS X Snow Leopard, CentOS 5.5 and Windows 7 with
VC++ 2008.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I'll take a look at the mailing lists. They're GNU projects, so
they're bound to have lists.

On 26 January 2011 13:57, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Personally, I expect zero warnings on any platform I work on. I
> currently test on OS X Snow Leopard, CentOS 5.5 and Windows 7 with
> VC++ 2008.

Alright, I'll bear that in mind. Now that I check against 2.8, I don't
even see those "ambiguous else" warnings that were so numerous when
building against 2.9. I suppose that we're now building with a
different warning level. This sort of illustrates my original point.

I don't know why warnings might differ between wx versions, because
-Wall is given for each build:

// 2.8
g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-2.9
-I/usr/local/include/wx-2.9 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2 -DDATA_DIR=\"/usr/local/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
dlgStep.o -MD -MP -MF .deps/dlgStep.Tpo -c -o dlgStep.o `test -f
'./agent/dlgStep.cpp' || echo './'`./agent/dlgStep.cpp

// 2.9
g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/local/pgsql/include
-I/usr/local/pgsql/include -DHAVE_CONNINFO_PARSE
-I/usr/local/lib/wx/include/gtk2-unicode-release-2.8
-I/usr/local/include/wx-2.8 -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES
-D__WXGTK__ -O2 -DEMBED_XRC -I/usr/include/libxml2
-I/usr/include/libxml2
-DDATA_DIR=\"/usr/local/pgadmin3/share/pgadmin3/\" -Wall
-Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include -MT
dlgStep.o -MD -MP -MF .deps/dlgStep.Tpo -c -o dlgStep.o `test -f
'./agent/dlgStep.cpp' || echo './'`./agent/dlgStep.cpp

I've fixed all warnings against 2.9 (except one tricky one).

Here's another warning of interest that I haven't been able to fix yet:

./gqb/gqbViewPanels.cpp: In member function ‘void
gqbGridPanel::OnButtonUp(wxCommandEvent&)’:
./gqb/gqbViewPanels.cpp:211:45: warning: operation on
‘((gqbGridPanel*)this)->gqbGridPanel::selTop’ may be undefined
./gqb/gqbViewPanels.cpp: In member function ‘void
gqbGridPanel::OnButtonDown(wxCommandEvent&)’:
./gqb/gqbViewPanels.cpp:292:45: warning: operation on
‘((gqbGridPanel*)this)->gqbGridPanel::selTop’ may be undefined
./gqb/gqbViewPanels.cpp: In member function ‘void
gqbOrderPanel::OnButtonUp(wxCommandEvent&)’:
./gqb/gqbViewPanels.cpp:1133:59: warning: operation on
‘((gqbOrderPanel*)this)->gqbOrderPanel::selRightTop’ may be undefined
./gqb/gqbViewPanels.cpp: In member function ‘void
gqbOrderPanel::OnButtonDown(wxCommandEvent&)’:
./gqb/gqbViewPanels.cpp:1214:59: warning: operation on
‘((gqbOrderPanel*)this)->gqbOrderPanel::selRightTop’ may be undefined

Basically, if you read a variable twice in an expression where it is
also written to, the result is undefined. See
http://www2.research.att.com/~bs/bs_faq2.html#evaluation-order for
more information. My problem here is that the evaluation order isn't
just undefined to the compiler :-)

I've changed my mind about fixing the Flex/Bison problem prior to
committing anything.

--
Regards,
Peter Geoghegan

I believe this warning is because something simple, try to verify that
pointer is set before using for evaluation of an expression, in other words
(example warning at gqb/gqbViewPanels.cpp:211)

(add if expression before using gModel...)

if(gModel) or if(gModel!=NULL) ....
gModel->changesPositions(selTop,selTop--);

that help you to verify that pointer is not null before using it. I don't
test/try this solution because at my compiler that warning is not shown, but
I believe is going to work.

Here's another warning of interest that I haven't been able to fix yet:
>
> ./gqb/gqbViewPanels.cpp: In member function ‘void
> gqbGridPanel::OnButtonUp(wxCommandEvent&)’:
> ./gqb/gqbViewPanels.cpp:211:45: warning: operation on
> ‘((gqbGridPanel*)this)->gqbGridPanel::selTop’ may be undefined
> ./gqb/gqbViewPanels.cpp: In member function ‘void
> gqbGridPanel::OnButtonDown(wxCommandEvent&)’:
> ./gqb/gqbViewPanels.cpp:292:45: warning: operation on
> ‘((gqbGridPanel*)this)->gqbGridPanel::selTop’ may be undefined
> ./gqb/gqbViewPanels.cpp: In member function ‘void
> gqbOrderPanel::OnButtonUp(wxCommandEvent&)’:
> ./gqb/gqbViewPanels.cpp:1133:59: warning: operation on
> ‘((gqbOrderPanel*)this)->gqbOrderPanel::selRightTop’ may be undefined
> ./gqb/gqbViewPanels.cpp: In member function ‘void
> gqbOrderPanel::OnButtonDown(wxCommandEvent&)’:
> ./gqb/gqbViewPanels.cpp:1214:59: warning: operation on
> ‘((gqbOrderPanel*)this)->gqbOrderPanel::selRightTop’ may be undefined
>
> Basically, if you read a variable twice in an expression where it is
> also written to, the result is undefined. See
> http://www2.research.att.com/~bs/bs_faq2.html#evaluation-order<http://www2.research.att.com/%7Ebs/bs_faq2.html#evaluation-order>for
> more information. My problem here is that the evaluation order isn't
> just undefined to the compiler :-)
>
>

Regards, Luis.

I've decided to take the program's actual behaviour on my system as an
indication of its intended behaviour in terms of the evaluation order
of arguments. So:

Breakpoint 4, gqbGridPanel::OnButtonUp (this=0x179fc10) at
./gqb/gqbViewPanels.cpp:208
208 allowSelCells = false;
(gdb) n
209 if((selTop >= 0 && selBottom == -1) || (selTop == selBottom))
(gdb) n
211 gModel->changesPositions(selTop, selTop--); // seltop is less
than selltop--
(gdb) n

Here's how I've decided to explicate that behaviour:

diff --git a/pgadmin/gqb/gqbViewPanels.cpp b/pgadmin/gqb/gqbViewPanels.cpp
index 3937753..d3087f8 100644
--- a/pgadmin/gqb/gqbViewPanels.cpp
+++ b/pgadmin/gqb/gqbViewPanels.cpp
@@ -208,7 +208,8 @@ void gqbGridPanel::OnButtonUp(wxCommandEvent &)
allowSelCells = false;
if((selTop >= 0 && selBottom == -1) || (selTop == selBottom))
{
- gModel->changesPositions(selTop, selTop--);
+ --selTop;
+ gModel->changesPositions(selTop, selTop + 1);
if(selTop < 0)
{
selTop = 0;
@@ -289,7 +290,8 @@ void gqbGridPanel::OnButtonDown(wxCommandEvent &)
// A single row is selected
if((selTop >= 0 && selBottom == -1) || (selTop == selBottom))
{
- gModel->changesPositions(selTop, selTop++);
+ ++selTop;
+ gModel->changesPositions(selTop, selTop - 1);

// Adjust selection when selected item it's last item.
if(selTop == gModel->GetNumberRows())
@@ -1130,7 +1132,8 @@ void gqbOrderPanel::OnButtonUp(wxCommandEvent &)
allowSelCells = false;
if((selRightTop >= 0 && selRightBottom == -1) || (selRightTop
== selRightBottom))
{
- tableRight->changesPositions(selRightTop, selRightTop--);
+ --selRightTop;
+ tableRight->changesPositions(selRightTop, selRightTop + 1);
if(selRightTop < 0)
{
selRightTop = 0;
@@ -1211,7 +1214,8 @@ void gqbOrderPanel::OnButtonDown(wxCommandEvent &)
// A single row is selected
if((selRightTop >= 0 && selRightBottom == -1) || (selRightTop
== selRightBottom))
{
- tableRight->changesPositions(selRightTop, selRightTop++);
+ ++selRightTop;
+ tableRight->changesPositions(selRightTop, selRightTop - 1);

// Adjust selection when selected item it's last item.
if(selRightTop == tableRight->GetNumberRows())

Attached patch fixes all warnings, and builds against wx 2.8 and 2.9,
or would were it not for OGL and Flex/Bison issues with 2.9.

--
Regards,
Peter Geoghegan

On Wed, Jan 26, 2011 at 6:41 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
>
> Attached patch fixes all warnings, and builds against wx 2.8 and 2.9,

Builds fine on my Mac. I wonder, should we use a macro to improve
readability, eg:

#define cstr(x) ((const wxChar *)x)

(can't use c_str - that seems to be in use already on VC++ 2008)

Also, did you change all of the variadic calls? I still see a
wxLogInfo with a .c_str() parameter in pgadmin/db/pgConn.cpp:169 for
example.

On VC++ 2008, I get the following warnings:

Warning 1 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\agent\pgajob.cpp 128
Warning 2 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\frm\frmhint.cpp 363
Warning 3 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\edbprivatesynonym.cpp 70
Warning 4 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgaggregate.cpp 189
Warning 5 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgaggregate.cpp 191
Warning 6 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgaggregate.cpp 193
Warning 7 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgcast.cpp 132
Warning 8 warning C4800: 'const wchar_t *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgforeignkey.cpp 233
Warning 9 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgserver.cpp 991
Warning 10 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgserver.cpp 1047
Warning 11 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgserver.cpp 1050
Warning 12 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgtable.cpp 961
Warning 13 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgtable.cpp 973
Warning 14 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgtable.cpp 1015
Warning 15 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\schema\pgtable.cpp 1044
Warning 16 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\slony\slcluster.cpp 298
Warning 17 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\slony\slcluster.cpp 309
Warning 18 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\slony\slnode.cpp 202
Warning 19 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\slony\slnode.cpp 246
Warning 20 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\slony\slnode.cpp 251
Warning 21 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\utils\syssettings.cpp 612
Warning 22 warning C4800: 'const wxChar *' : forcing value to bool
'true' or 'false' (performance
warning) c:\users\dpage\documents\pgadmin3\pgadmin\debugger\dlgdirectdbg.cpp 365

> or would were it not for OGL and Flex/Bison issues with 2.9.

Whats the plan with OGL? I think we should get a list of all the
committers to it (excluding the bakefile, which we don't need), and
email them, explaining that Julian has given permission to relicence
the code, and asking if they'll be kind enough to do the same.

Thoughts? Do you want to do that, or should I?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 29 January 2011 09:30, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> Builds fine on my Mac. I wonder, should we use a macro to improve
> readability, eg:
>
> #define cstr(x) ((const wxChar *)x)
>
> (can't use c_str - that seems to be in use already on VC++ 2008)

c_str() is the member function that std::string clients use when
interfacing with C. The convention has inspired the wxWidgets
developers, who have a long term goal to move to using std::string,
but it has also inspired, for example, libpqxx, where there is a
c_str() member function of a proxy class that encapsulates single
datums from result sets. I think that this exposes us to problems in
the future. We're following the documented way of getting 2.8 and 2.9
compatibility here. I generally dislike using macros for things like
this when working in C++.

> Also, did you change all of the variadic calls? I still see a
> wxLogInfo with a .c_str() parameter in pgadmin/db/pgConn.cpp:169 for
> example.

No, I didn't. I left it up to my compiler. Note that the wx built-in
functions like wxLogInfo are defined using macros to construct each
variant in /wx/log.h . The calls I fixed are to variants of our own,
similarly defined using macros in sysLogger.h, but with a different
function signature to the 2.9 wx ones. Ours (which is behaviourally
identical to wx 2.8's) is:

extern void wxLog##level(const wxChar *szFormat, ...)
COMPAT_ATTRIBUTE_PRINTF_1 // remember I created the compatibility
macro COMPAT_ATTRIBUTE_PRINTF_1?

In Wx's 2.9's things are more complex. Here is a comment from that header:

/*
The code below is unreadable because it (unfortunately unavoidably)
contains a lot of macro magic but all it does is to define wxLogXXX() such
that you can call them as vararg functions to log a message at the
corresponding level.

More precisely, it defines:

- wxLog{FatalError,Error,Warning,Message,Verbose,Debug}() functions
taking the format string and additional vararg arguments if needed.
- wxLogGeneric(wxLogLevel level, const wxString& format, ...) which
takes the log level explicitly.
- wxLogSysError(const wxString& format, ...) and wxLogSysError(long
err, const wxString& format, ...) which log a wxLOG_Error severity
message with the error message corresponding to the system error code
err or the last error.
- wxLogStatus(const wxString& format, ...) which logs the message into
the status bar of the main application window and its overload
wxLogStatus(wxFrame *frame, const wxString& format, ...) which logs it
into the status bar of the specified frame.
- wxLogTrace(Mask mask, const wxString& format, ...) which only logs
the message is the specified mask is enabled. This comes in two kinds:
Mask can be a wxString or a long. Both are deprecated.

In addition, wxVLogXXX() versions of all the functions above are also
defined. They take a va_list argument instead of "...".
*/

Obviously the wx guys were aware of the need to change their
implementation to prevent the c_str() proxy ambiguity thing happening
to their clients. What do you want to do about it? I'm not aware of
there being an official way to extend the wx logger functionality.

> On VC++ 2008, I get the following warnings:
>
> Warning 1       warning C4800: 'const wxChar *' : forcing value to bool
> 'true' or 'false' (performance

Oh yeah, I know the C4800 warning well. I'm glad to see it, because it
has revealed that I've introduced a bug, which nicely answers my
earlier question of "why does sysSettings::Write(const wxString&,
const wxChar*) take a const wxChar* as its second argument rather than
just a wxString?". We're calling the wrong overload where we don't
supply a wxString directly, because C++ prefers converting to a
built-in type to converting using a class constructor with a wxChar*
argument. I'm going to change the bool overloads and perhaps others to
functions with distinct names. Things are obviously too brittle as
they stand. Objections?

>> or would were it not for OGL and Flex/Bison issues with 2.9.
>
> Whats the plan with OGL? I think we should get a list of all the
> committers to it (excluding the bakefile, which we don't need), and
> email them, explaining that Julian has given permission to relicence
> the code, and asking if they'll be kind enough to do the same.
>
> Thoughts? Do you want to do that, or should I?

I imagined that you wanted to use your legal resources to make sure
that it was done the right way, but I don't mind doing the ground
work. Will I go ahead with it? The problem with asking me to do it is
that I will split hairs to be on the very safe side, which may not be
necessary. I suppose I can just consult you if I hit a snag.

--
Regards,
Peter Geoghegan

On Sat, Jan 29, 2011 at 6:08 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> We're following the documented way of getting 2.8 and 2.9
> compatibility here. I generally dislike using macros for things like
> this when working in C++.

Similarly, I dislike uglifying code if we can help it - and this seems
like a common enough case to me that a macro isn't the end of the
world to improve readability.

What do others think?

>> Also, did you change all of the variadic calls? I still see a
>> wxLogInfo with a .c_str() parameter in pgadmin/db/pgConn.cpp:169 for
>> example.
>
> No, I didn't. I left it up to my compiler. Note that the wx built-in
> functions like wxLogInfo are defined using macros to construct each
> variant in /wx/log.h . The calls I fixed are to variants of our own,
> similarly defined using macros in sysLogger.h, but with a different
> function signature to the 2.9 wx ones. Ours (which is behaviourally
> identical to wx 2.8's) is:
>
> extern void wxLog##level(const wxChar *szFormat, ...)
> COMPAT_ATTRIBUTE_PRINTF_1 // remember I created the compatibility
> macro COMPAT_ATTRIBUTE_PRINTF_1?

Ahh, yes.

> Obviously the wx guys were aware of the need to change their
> implementation to prevent the c_str() proxy ambiguity thing happening
> to their clients. What do you want to do about it? I'm not aware of
> there being an official way to extend the wx logger functionality.

No, I don't think there is. I guess we'll need to replicate the 2.9
signatures/macros if we're building against 2.9. Ugly, but it should
be fairly self-contained.

>> On VC++ 2008, I get the following warnings:
>>
>> Warning 1       warning C4800: 'const wxChar *' : forcing value to bool
>> 'true' or 'false' (performance
>
> Oh yeah, I know the C4800 warning well. I'm glad to see it, because it
> has revealed that I've introduced a bug, which nicely answers my
> earlier question of "why does sysSettings::Write(const wxString&,
> const wxChar*) take a const wxChar* as its second argument rather than
> just a wxString?". We're calling the wrong overload where we don't
> supply a wxString directly, because C++ prefers converting to a
> built-in type to converting using a class constructor with a wxChar*
> argument. I'm going to change the bool overloads and perhaps others to
> functions with distinct names. Things are obviously too brittle as
> they stand. Objections?

Urgh. But I suppose we'll have to.

>>> or would were it not for OGL and Flex/Bison issues with 2.9.
>>
>> Whats the plan with OGL? I think we should get a list of all the
>> committers to it (excluding the bakefile, which we don't need), and
>> email them, explaining that Julian has given permission to relicence
>> the code, and asking if they'll be kind enough to do the same.
>>
>> Thoughts? Do you want to do that, or should I?
>
> I imagined that you wanted to use your legal resources to make sure
> that it was done the right way,

Our legal resources are, umm, overstretched to say the least. I'm not
sure they'd tell us anything other than to get the authors' permission
anyway.

> but I don't mind doing the ground
> work. Will I go ahead with it? The problem with asking me to do it is
> that I will split hairs to be on the very safe side, which may not be
> necessary. I suppose I can just consult you if I hit a snag.

Well, how about starting by figuring out who we need to contact (and
let's keep that private). Then, between us we can draft an email to
sent to those people, and if any of them respond in a less than clear
fashion, we can figure out how to deal with them individually.

Sound OK?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 31 January 2011 13:02, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> Similarly, I dislike uglifying code if we can help it - and this seems
> like a common enough case to me that a macro isn't the end of the
> world to improve readability.

Bear in mind that we don't have to live with those casts forever -
when we drop 2.8 support, we can refactor. Are you sure that it's
actually possible to have the preprocessor replace a call to a member
function on an object with a cast like that? I've never seen that
before, and I couldn't produce a working testcase.

>> Obviously the wx guys were aware of the need to change their
>> implementation to prevent the c_str() proxy ambiguity thing happening
>> to their clients. What do you want to do about it? I'm not aware of
>> there being an official way to extend the wx logger functionality.
>
> No, I don't think there is. I guess we'll need to replicate the 2.9
> signatures/macros if we're building against 2.9. Ugly, but it should
> be fairly self-contained.

Very ugly. That self-described "unreadable" code makes my eyes glaze
over. I suppose it's unavoidable, given that the problem of the proxy
not being trivially constructible to pass to variadics isn't going
anywhere, but the casts are expected to be removed eventually.

>>> On VC++ 2008, I get the following warnings:
>>>
>>> Warning 1       warning C4800: 'const wxChar *' : forcing value to bool
>>> 'true' or 'false' (performance
>>
>> Oh yeah, I know the C4800 warning well. I'm glad to see it, because it
>> has revealed that I've introduced a bug, which nicely answers my
>> earlier question of "why does sysSettings::Write(const wxString&,
>> const wxChar*) take a const wxChar* as its second argument rather than
>> just a wxString?". We're calling the wrong overload where we don't
>> supply a wxString directly, because C++ prefers converting to a
>> built-in type to converting using a class constructor with a wxChar*
>> argument. I'm going to change the bool overloads and perhaps others to
>> functions with distinct names. Things are obviously too brittle as
>> they stand. Objections?
>
> Urgh. But I suppose we'll have to.

Here's how I see it. We're not actually appending a bool in the case
of ctlListView - we're appending a "Yes" or a "No", so a different
function name is actually justified. I have no such justification for
sysSettings, but on the other hand, there seems to be no justification
for the bool overload of write there to even exist (it isn't called),
so I've just removed it. Should I have followed what I did for
ctlListView?

On the plus side, calls to ctlListView::AppendItem(const wxString&,
bool) now don't compile at all, so no one is going to accidentally
re-introduce the bug by using the old convention. Also, I haven't had
to change any of the other overloads, so changes made aren't so
disruptive.

>> but I don't mind doing the ground
>> work. Will I go ahead with it? The problem with asking me to do it is
>> that I will split hairs to be on the very safe side, which may not be
>> necessary. I suppose I can just consult you if I hit a snag.
>
> Well, how about starting by figuring out who we need to contact (and
> let's keep that private). Then, between us we can draft an email to
> sent to those people, and if any of them respond in a less than clear
> fashion, we can figure out how to deal with them individually.
>
> Sound OK?

I'm happy with that. I agree with the need to keep something that
sensitive off-list. I will gather those names.

I would like to see us tie up all the loose ends with the work done to
date and to get it committed. I feel that we need to draw a line under
it before really moving on to OGL and the Flex/Bison problem.

--
Regards,
Peter Geoghegan

On Mon, Jan 31, 2011 at 3:35 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> On 31 January 2011 13:02, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> Similarly, I dislike uglifying code if we can help it - and this seems
>> like a common enough case to me that a macro isn't the end of the
>> world to improve readability.
>
> Bear in mind that we don't have to live with those casts forever -
> when we drop 2.8 support, we can refactor. Are you sure that it's
> actually possible to have the preprocessor replace a call to a member
> function on an object with a cast like that? I've never seen that
> before, and I couldn't produce a working testcase.

I tried it on VC++ and it didn't complain. Didn't test gcc though.

> Here's how I see it. We're not actually appending a bool in the case
> of ctlListView - we're appending a "Yes" or a "No", so a different
> function name is actually justified. I have no such justification for
> sysSettings, but on the other hand, there seems to be no justification
> for the bool overload of write there to even exist (it isn't called),
> so I've just removed it. Should I have followed what I did for
> ctlListView?

If it's not used, it can go. With some of the early utility classes we
wrote variations of member functions that we thought we were going to
use, as well as ones that we once did use, but since have become
obsolete.

> I'm happy with that. I agree with the need to keep something that
> sensitive off-list. I will gather those names.

Thanks.

> I would like to see us tie up all the loose ends with the work done to
> date and to get it committed. I feel that we need to draw a line under
> it before really moving on to OGL and the Flex/Bison problem.

OK - please send the updated patch and I'll test it again.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 31 January 2011 20:27, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> I tried it on VC++ and it didn't complain. Didn't test gcc though.

Can you produce a test case? I wasn't aware that it is possible to use
the preprocessor to define C++ member function macros.

> If it's not used, it can go.

Good, done.

>> I would like to see us tie up all the loose ends with the work done to
>> date and to get it committed. I feel that we need to draw a line under
>> it before really moving on to OGL and the Flex/Bison problem.
>
> OK - please send the updated patch and I'll test it again.

Will do.

--
Regards,
Peter Geoghegan

On Tue, Feb 1, 2011 at 12:26 AM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> On 31 January 2011 20:27, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> I tried it on VC++ and it didn't complain. Didn't test gcc though.
>
> Can you produce a test case? I wasn't aware that it is possible to use
> the preprocessor to define C++ member function macros.

I was suggesting to use it instead of the ugly cast, not to define a
member function - eg.

diff --git a/pgadmin/pgAdmin3.cpp b/pgadmin/pgAdmin3.cpp
index 603c0fd..1b7105f 100644
--- a/pgadmin/pgAdmin3.cpp
+++ b/pgadmin/pgAdmin3.cpp
@@ -311,10 +311,11 @@ bool pgAdmin3::OnInit()
// Setup logging
InitLogger();

+#define cstr(x) ((const wxChar *)x)
wxString msg;
msg << wxT("# ") << appearanceFactory->GetLongAppName() <<
wxT(" Version ") << VERSION_STR << wxT(" Startup");
wxLogInfo(wxT("##############################################################"));
- wxLogInfo(wxT("%s"), msg.c_str());
+ wxLogInfo(wxT("%s"), cstr(msg));
wxLogInfo(wxT("##############################################################"));

#ifdef SSL

(which works fine on GCC too it seems).

>> OK - please send the updated patch and I'll test it again.
>
> Will do.

Thanks.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I now use c_str() for logging variadics without casting, extending wx
logging in a way that provides the same interface as the existing 2.8
code.

There are still two areas where we still cast for variadics, where I
didn't want to copy the black magic that wx does to make the proxy
passable. Here's one:

report->XmlAddSectionTableRow(section,
column->GetColNumber(),
6,
(const wxChar*) colName,
(const wxChar*) column->GetVarTypename(),
(const wxChar*) BoolToYesNo(column->GetNotNull()),
(const wxChar*) BoolToYesNo(column->GetIsPK()),
(const wxChar*) column->GetDefault(),
(const wxChar*) column->GetComment());

I think that to copy the wx technique verbatim would be a net gain in
ugliness, because we're currently simply naive clients of it.

I've performed the changes to overloads of sysSettings::Write() and
ctlListView::AppendItem() already discussed.

Now, sysLogger.h looks like this:

#define wxLOG_Notice (wxLOG_User+1)
#define wxLOG_Sql (wxLOG_User+2)
#define wxLOG_QuietError (wxLOG_User+3)
#define wxLOG_Script (wxLOG_User+4)
#define wxLOG_ScriptVerbose (wxLOG_User+5)

#if wxCHECK_VERSION(2, 9, 0)

#define wxLogNotice wxDO_LOG(Notice)
#define wxLogSql wxDO_LOG(Sql)
#define wxLogQuietError wxDO_LOG(QuietError)
#define wxLogScript wxDO_LOG(Script)
#define wxLogScriptVerbose wxDO_LOG(ScriptVerbose)

#else
#define DECLARE_INT_LOG_FUNCTION(level) \
extern void wxVLog##level(const wxChar *szFormat, va_list argptr); \
extern void wxLog##level(const wxChar *szFormat, ...) ATTRIBUTE_PRINTF_1

DECLARE_INT_LOG_FUNCTION(Notice);
*** SNIP*** // same as before
#endif

This works fine for client TUs. However, sysLogger.cpp doesn't compile
on 2.9 . I don't know why we've provided implementations of various
logger functions there. What do you think? I've attached a patch of my
latest revisions so you can take a look yourself.

By the way, reading 2.9 log.h I learned something about the "ambiguous
else" warnings:

// work as expected, without it the second "else" would match the "if"
// inside wxLogError(). Unfortunately code like
//
// if ( cond )
// wxLogError("!!!");
//
// now provokes "suggest explicit braces to avoid ambiguous 'else'"
// warnings from g++ 4.3 and later with -Wparentheses on but they can be
// easily fixed by adding curly braces around wxLogError() and at least
// the code still does do the right thing.

I'm not sure that your suggestion about the cstr() macro is a win, but
if we do it I feel that it should be an inline function.

--
Regards,
Peter Geoghegan

First off - patch applied, thanks!

On Tue, Feb 1, 2011 at 3:24 PM, Peter Geoghegan
<peter(dot)geoghegan86(at)gmail(dot)com> wrote:
> I've performed the changes to overloads of sysSettings::Write() and
> ctlListView::AppendItem() already discussed.

I've reverted the removal of this function:

// Write a boolean value
bool sysSettings::Write(const wxString &key, bool value)
{
return Write(key, BoolToStr(value));
}

It was being used (probably for historical reasons) to store values
such as the "frmMain/Maximised". Removing it causes use to write those
values on Windows as DWORD registry keys, which then asserts when we
try to read the value at startup.

I also changed this line (363) in frmHint.cpp:

settings->Write(wxString(wxT("Hints/")) + hintArray[hintno].hintPage,
wxString(wxEmptyString));

It was using a plain wxEmptyString for the last argument, which without:

bool Write(const wxString &key, const wxChar *value)

meant that the wxEmptyString was getting case to a boolean.

Quick fixes to ensure the code works as previously - feel free to
clean it up :-)

> This works fine for client TUs. However, sysLogger.cpp doesn't compile
> on 2.9 . I don't know why we've provided implementations of various
> logger functions there. What do you think? I've attached a patch of my
> latest revisions so you can take a look yourself.

Well the reason is simply that out logging infrastructure has
additional requirements that wxWidgets doesn't provide - additional
logging levels for example.

As for the problem - I'm not currently in a position to test with 2.9,
so cannot really comment. What is the problem/error you see exactly?

> I'm not sure that your suggestion about the cstr() macro is a win, but
> if we do it I feel that it should be an inline function.

I can live without it.

Let's continue this on a new thread...

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company