incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Travis Vitek <Travis.Vi...@roguewave.com>
Subject RE: STDCXX-1066 [was: Re: STDCXX forks]
Date Mon, 24 Sep 2012 16:28:37 GMT
Comments below...

> -----Original Message-----
> From: Stefan Teleman
> Sent: Monday, September 24, 2012 5:46 AM
> Subject: Re: STDCXX-1066 [was: Re: STDCXX forks]
> 
> On Mon, Sep 24, 2012 at 8:21 AM, Liviu Nicoara <nikkoara@hates.ms>
> wrote:
> 
> > In the light of your inability to answer the simplest questions about
> > the correctness and usefulness of this patch, I propose we strike the
> > patch in its entirety.
> 
> Let me make something very clear to you: what I am doing here is a
> courtesy to the stdcxx project.

Thank you for your contribution. There is a good chance that without your activity on this
project over the last few months that it would be in the attic.

> There is no requirement in my job
> description to waste hours arguing with you in pointless squabbles
> over email. Nor is there a requirement in the APL V2.0 which would
> somehow compel us to redistribute our source code changes.

The problem here is that code changes are expected to pass review. They must follow established
conventions, solve the problem in a reasonable and portable way, provide a test case (where
one doesn't already exist), as well as make sense to everyone who is working with the product.

Several of the changes included don't make sense to the rest of us. Unless we manage to figure
them out on our own or you manage to explain them to us, then the changes should probably
be excluded from the code.

For example, the changes to src/exception.cpp don't make sense to me. Here is the relevant
diff...

  +#if defined(_RWSTD_STRICT_SPARCV8_MUTEX_ALIGNMENT)
  +#  pragma pack(8)
  +#  pragma align 8(__rw_aligned_buffer)
  +#endif
  +
       // facet must never be destroyed
       static __rw_aligned_buffer<_Msgs> msgs;

It seems that we trying to give 8-byte alignment to a class of type __rw_aligned_buffer. The
point of __rw_aligned_buffer is to give us a block of properly aligned data, and if it isn't
aligned, then it is broken. So, why are we using a pragma for alignment here (and potentially
in other places) when it seems that there is a bug in __rw_aligned_buffer that we should be
addressing? Of course, if this is a problem with aligned buffer, we need a testcase.

Another example are the changes to src/ctype.cpp

  +#if defined(_RWSTD_STRICT_SPARCV8_MUTEX_ALIGNMENT)
  +#  pragma pack(8)
  +#  pragma align 8(f)
  +#  pragma align 8(pf)
  +#endif
           static union {
  +#if defined(_RWSTD_STRICT_SPARCV8_MUTEX_ALIGNMENT)
  +            unsigned long long align_;
  +            unsigned char data_ [sizeof (_STD::ctype<char>)];
  +#else
               void *align_;
               char  data_ [sizeof (_STD::ctype<char>)];
  +#endif
           } f;
           static __rw_facet* const pf =
               new (&f) _STD::ctype<char>(__rw_classic_tab, false, ref);
           pfacet = pf;
  +#if defined(_RWSTD_STRICT_SPARCV8_MUTEX_ALIGNMENT)
  +#  pragma pack(0)
  +#endif

This change seems to give alignment to `f' in two different ways, and it seems to be applying
alignment to the pointer (the issue Liviu has brought up). It seems like the simpler fix would
be to unconditionally add union members to `f' so that we get the proper alignment (much like
we try to do in __rw_aligned_buffer), or an even better solution would be to use __rw_aligned_buffer.

> 
> > We could and should re-work the instances in the library where
> > we might use mutex and condition objects that are misaligned wrt the
> > mentioned kernel update.
> 
> Yeah, why don't you go ahead and do that. Just like you fixed stdcxx-
> 1056.
> 

There is no need for comments like this.

Stefan, if you are feeling singled out and attacked because your patches aren't accepted without
question, it might be a good idea to go back and look through the mailing list archives. We
have _all_ been in this position. Nearly every time I posted a patch for the first year working
on stdcxx was like this, and I've been doing C++ software for 15 years.

The company where I work currently has a review process that is just as rigorous as what you're
seeing with stdcxx now. The process isn't there to attack anyone personally, but to ensure
the quality and maintainability of the library for years to come.

Travis


Mime
View raw message