Re: wxWidgets 2.9 build

From: Peter Geoghegan <peter(dot)geoghegan86(at)gmail(dot)com>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: Magnus Hagander <magnus(at)hagander(dot)net>, pgadmin-hackers(at)postgresql(dot)org
Subject: Re: wxWidgets 2.9 build
Date: 2011-01-29 18:08:03
Message-ID: AANLkTinNqjN25cZw0eKA1HByh7bTbiXYCTLRru-18=Ba@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

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

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Guillaume Lelarge 2011-01-30 15:55:14 pgAdmin III commit: Add a specific panel for selection information
Previous Message Dave Page 2011-01-29 09:30:42 Re: wxWidgets 2.9 build