On Tue, 2013-01-29 at 20:19 +0100, Andi Kleen wrote:
> > next time please reply to the points raised in a review if you disagree
> > with them, and don't just ignore them. That speeds up the review.
>
> It was all in the previous email on the topic.
This v2 patch did not incorporate all the changes that I requested, nor
did you explicitly object to those you didn't incorporate. Maybe you
don't care what's in libitm's comments, but I do.
> > > // See gtm_thread::begin_transaction.
> > > -uint32_t GTM::htm_fastpath = 0;
> > > +uint32_t GTM::htm_fastpath asm("__gtm_htm_fastpath") = 0;
> > > +
> > > +uint32_t *GTM::global_lock asm("__gtm_global_lock");
> >
> > Rearrange the serial lock's fields (see below).
>
> To my knowledge C++ classes have no guaranteed layout,
> so that's not safe because there is no guarantee where
> the vtable pointers are. it would be only with plain old structs.
And gtm_rwlock doesn't have any virtual members, right? So it's like a
plain old struct (ie, it's a POD type).
> + // pr_tryHTM can be set by an assembler fast path when it already
> tried
> + // a hardware transaction once. In this case we do one retry less.
>
> pr_tryHTM is already documented.
And I asked you to simply reference this in a comment. What's the
problem with that? I do not want people working on libitm to have to
grep through the code for uses of pr_tryHTM and look for comments that
might be related to it just because you don't want to put a simple
reference into the comment at the declaration of pr_tryHTM. Can't you
see that adding the reference is just plain useful for everybody else?
Torvald
