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 <[email protected]>
> 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
