This is the mail archive of the
cygwin-patches
mailing list for the Cygwin project.
Re: [PATCH] Libstdc++ support changes.
On Tue, Jul 07, 2009 at 07:18:58PM +0200, Corinna Vinschen wrote:
>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.
Nope. I noticed the comment style too (and missed the two spaces after
the colon). Obviously not a big deal but it would be nice to have them
consistent.
cgf