Re: [Swig-devel] generated PHP destructors & smart pointers

[Olly B. <ol...@su...>]

On 2009-06-29, David Fletcher <fr...@tu...> wrote:
> I'm trying to get boost's intrusive_ptr work going with PHP.  Most of
> the work is done outside of the swig's (C++) code generator, but I
> have had to make one change to Source/Modules/php.cxx so that the
> generated destructors would play nice with smart pointers.
>
> I thought I'd send along the changes I've made to php.cxx so that
> others could have a look the code.  The diffs are against the latest
> version of php.cxx (on the main branch).

I don't know anything about the smartptr stuff, but the idea of iterating
upwards to look for it seems unwise to me.  It's additional overhead whenever
there isn't a smartptr involved, which is most of the time for most users.

It also seems it's at least as likely to cause problems as be future proof
against changes to how smartptr is used.

It's better to be consistent with how existing backends with smartptr support
behave (Python is generally a good one to look at of the scripting languages
as it tends to be in good shape), and then PHP is more likely to work or break
in the same ways.  And we can defend against possible future breakage by
providing good test coverage.

> There are a couple of 'TODO's in the code.  If anyone has any comments
> on these, I'd be grateful to see them.

I don't know about the TODOs, but I'd suggest that the code in python.cxx which
handles smartptr might be worth a read.

> (And, yes, I know, my indentation style doesn't match what's used in
> swig.  I'll change this over before submitting the official patch.)

Wouldn't it just be simpler to follow the existing indentation style from the
start?  But whatever you prefer so long as the final patch is good.

BTW, please try to avoid formatting changes unrelated to the change you're
making as that makes it harder to review (some existing comments have been
reformatted and punctuation added, and you seem to have wrapped a long
line in one place, or at least if it's not just been wrapped, I've failed
to spot the change after wasting a minute staring at it...)

If stuff needs reformatting, that's OK, but please make it a separate patch.

Cheers,
    Olly