Re: [Xen-devel] [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 15 Nov 2018, Julien Grall wrote:
> Hi,
> 
> On 11/15/18 1:19 PM, Andrii Anisov wrote:
> > > So I would prefer to stick with _t which is quite common within the p2m
> > > code base so far.
> > 
> > I've found a similar code only in one place - p2m_get_entry()
> > function. And it is, at least, somehow commented there:
> > ...
> >      /* Allow t to be NULL */
> >      t = t ?: &_t;
> > 
> >      *t = p2m_invalid;
> > ...
> 
> And in x86 code...
> 
> > 
> > But IMO, it is really confusing to write a code to calculate and store
> > a value which clearly is not needed by a caller.
> 
> I disagree, you directly know that t can be NULL. Although, I agree that a
> comment would help to understand.
> 
> >  From another hand, I'm not sure if a compiler would be intelligent
> > enough to factor out the odd code from execution pass on the incoming
> > null pointer.
> 
> You can't assume the compiler will inline even with the inline keyword.
> 
> > 
> > I'm sorry, but I can't pass my RB for `_t`.
> 
> I think this request is unreasonable because this is a matter of taste.
> 
> While this is a nice feature to have in Xen 4.12 and address a long standing
> open issue on Arm. I don't require it and I am not inclined to address
> bikeshedding.
> 
> So I will keep aside for now until someone cares about it.

Let's try to be reasonable and have constructive conversations. If we do
have to disagree, let's disagree on meaningful design decisions, not
silly code style issues :-)

Andrii, when something can be done equally well in two different ways,
especially when it is a matter of style, I think it is best to express
our preference, but not to force a contributor in a particular
direction, unless specifically stated in CODING_STYLE or equivalent
docs. We don't have written rules about code reviews, but if we had,
I think this should be one of them. (I would love to have written rules
about code reviews.)

Julien, usually in situations like this, it is quicker to change the
code than to argue. I personally don't care and wouldn't ask you to
change it, but if a member of the community reads the code and has an
adverse reaction, it is always worth reconsidering the approach, because
others might find it antagonizing too. Given the choice, it is best to
go for something obvious to most, so if a new contributor sends a patch
to change, it is more likely to be correct from the start.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel