Re[2]: [Gc] Round 3 or 4 of C++ throw/nothrow patch.

Ivan Maidanski ivmai at mail.ru
Fri Dec 18 06:50:12 PST 2009


Hi!
"Talbot, George" <Gtalbot at ansarisbio.com> wrote:
> Revised patch attached, see responses inline.
> ...
> >
> > My miscellaneous comment about the patch:
> > 1. please use "-ru[N]" option for diff.
>
> Done.  See attached.
>
> > 2. please wrap long cod lines (longer than 78 chars).
>
> Fixed.

For configure.ac too.

>
> > 3. cxxabi.h seems to be missing in some compiler suites (so, I can't
> > compile on VC++, right?) - at least non-portable things should be guarded
> > with the config macro;
>
> They were for something else and no longer needed.  Took them out.  Thanks for catching this.

Nonetheless, the patch breaks C++ support for some compilers (I observe).

Could you guard include new/stdexcept and the rest code (which can't be compiled without that include files) with a macro (also briefly explain (in a code comment) which things are imported from each file respectively). I also think (but can't test this right now) that some compilers may signal error in the declaration like, eg., "void* operator new( size_t size, const std::nothrow_t& ) throw()" because this is a built-in operator (eg., some a compiler may expect it to be declared as "void* operator new(size_t)"). I'm not a guru in C++ dialects/standards but you can see a lot of tuning ifdef's in gc_cpp.h and you suggest a more complex declarations w/o a single ifdef (GC_NEW_COLLECTABLE is not related to the declarations).

>
> > 4. as for gc_cpp_init I think it's better to remove GC_INIT() (let it be
> > the client responsibility to call it) and rename it to something like
> > GC_setup_cpp_oom().
>
> Done.

gc_cpp_oom_fn() should have static linkage.

GC_setup_cpp_oom() should, IMHO, be a C symbol (according to the rule "the simple - the better", recall that C++ names are mangled differently by different compilers) and be defined as:

extern "C" GC_API void GC_CALL GC_setup_cpp_oom(void) {
 GC_set_oom_fn(gc_cpp_oom_fn);
}

Also, it's good to add to the comment something like "Called by the client after GC_INIT()".

>
> > 5. Since we are really having only main trunk at BDWGC CVS, only bugfix
> > patches are applied up to the final 7.2 release.
>
> Would this qualify as an exception if it doesn't change the interface or default behavior?  I've been trying to get this in for a while...
>
> > 6. the "__GNUC__ < 2 || __GNUC__ == 2..." in gc_cpp.cc has proper
> > indentation (unlike that in your patch - as I recall I've already pointed
> > to it in your previous patch). Also please clean up a bit your patch and
> > remove the "no-change" fragments like:
> >
> > <     A* a = new A;       // a is collectable.
> > ---
> > >     A* a = new A;       // a is collectable.[spaces here]
>
> Fixed.

Not in all places, try to call diff with and w/o --ignore-space-changes/tab-expansion and see the difference (really not a big deal but makes the patch smaller to review). (Also no need to put auto-gen files like config.h.in in the patch.)

>
> > 7. Optionally it might be good (but I don't have a strong opinion here) to
> > have the possibility of choosing between GC_MALLOC and
> > GC_MALLOC_UNCOLLECTABLE from the client code at start-up (e.g export
> > GC_set_new_collectible(0/1)).
>
> If I recall correctly, Hans Boehm was worried about the overhead, so that's why I did this as a compile option.  I'm not averse to changing this to runtime-chooseable if necessary, though it'd be hard to call GC_set_new_collectible() before all constructors of static objects run.

Ok. (but this not universal - consider you have an installed libgc.so and 2 C++ applications, one of which expects new operator to use GC_MALLOC_UNCOLLECTABLE and the other one not. If there were a tuning function call then there wouldn't be a problem... Any idea?)

>
> > 8. gc_cpp_oom_fn() should be tagged with GC_CALLBACK.
>
> Done.
>
> > 9. I think newly-added exportable functions should have "GC_" prefix
> > (instead of "gc_").
>
> Done.

10. Could you explain (at least to me) what's the purpose of the following addition?

+    catch (...)
+    {
+        return 0;
+    }

>
> > I'm intentionally not commenting the functionality of the added feature -
> > Hans knows better (the dark sides of the C++ part of GC).
>
> I'm hoping with fingers crossed that this is non-intrusive enough...
>
> Thanks for taking the time to look at the patch.

Try to refine the patch.

Bye.


More information about the Gc mailing list