Re: [PATCH] Libstdc++ support changes.

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Dave,

Thanks for doing that stuff!

On Jul  7 17:22, Dave Korn wrote:
> 
>     Hi all,
> 
>   I just got done doing a C/C++/libstdc++-v3 test run against GCC HEAD using
> the Cygwin DLL built with these patches, and everything worked.  In
> particular, it passed these tests:
> 
> > FAIL: g++.old-deja/g++.abi/cxa_vec.C execution test
> > FAIL: g++.old-deja/g++.brendan/new3.C execution test
> 
> ... which fail on current 4.3.2-2 using shared libstdc++ DLL precisely because
> they expect to be able to interpose libstdc++'s own internal calls to the
> allocation operators.  I've also been using it in daily use (and before that,
> the previous spin of this patch) for a while now and nothing unusual has been
> showing up.
> [...]

This looks pretty good to me.  I have just two formal nits.

In the ChangeLogs,

> 	* Makefile.common (COMPILE_CXX):  Add support for per-file overrides

please use just one space after the colon.

At some points you're using different comment types rather freely.
Here's an example.

> Index: winsup/cygwin/libstdcxx_wrapper.cc
> ===================================================================
> RCS file: winsup/cygwin/libstdcxx_wrapper.cc
> diff -N winsup/cygwin/libstdcxx_wrapper.cc
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ winsup/cygwin/libstdcxx_wrapper.cc	7 Jul 2009 15:21:57 -0000
> @@ -0,0 +1,91 @@
> +/* libstdcxx_wrapper.cc
> +
> +   Copyright 2009 Red Hat, Inc.
> +
> +This file is part of Cygwin.
> +
> +This software is a copyrighted work licensed under the terms of the
> +Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
> +details.  */

^^^^^
That's ok.

> +
> +
> +/* We provide these stubs to call into a user's
> +   provided ONDEE replacement if there is one - otherwise
> +   it must fall back to the standard libstdc++ version.
> +*/

^^^^^
The comment closing */ should be at the end of the last line of comment,
rather than starting a new line.

> +#include "winsup.h"
> +#include "cygwin-cxx.h"
> +#include "perprocess.h"
> +
> +// We are declaring asm names for the functions we define here, as we want
> +// to define the wrappers in this file.  GCC links everything with wrappers
> +// around the standard C++ memory management operators; these are the wrappers,
> +// but we want the compiler to know they are the malloc operators and not have
> +// it think they're just any old function matching 'extern "C" _wrap_*'.

^^^^^
While we have a couple of // comments in Cygwin, it would be nice to at
least don't use them for multiline comments and comments on their own
line.  Use

  /* This is a comment. */
  /* This is
     another comment. */

instead.

Other than that it looks like you tested this a lot so it's fine with
me.  Maybe Chris has some additional comment.


Corinna


-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat