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-31 15:35:32
Message-ID: AANLkTi=cjuafvvbOH9PRjvugw3CQmHdh1E88kcaoYBHY@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

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

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message pgAdmin Trac 2011-01-31 18:26:37 [pgAdmin III] #300: Add support for ALTER TABLE ADD UNIQUE/PRIMARY KEY USING INDEX.
Previous Message Nikhil S 2011-01-31 13:32:54 Re: pgAdmin III: EDB procedures with non void return types not handled properly