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 Stefano,
> 
> On 11/15/18 6:44 PM, Stefano Stabellini wrote:
> > 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.
> 
> I agree with your point here but this is also valid in the other way. If the
> suggested alternative provokes an adverse reaction to the contributor, then
> why should he use it?
> 
> As I wrote earlier one trying to use ternary condition split over 2 lines is
> not making the code more obvious. So I am not entirely sure why I should be
> forced to write such code?

I don't think you should be. You can find another way. For instance, the
whole thing could be reduce to one more "if" condition:

  if ( t != NULL )
  {
      *t = p2m_invalid;
      if ( page->u.inuse.type_info & PGT_writable_page )
          *t = p2m_ram_rw;
      else
          *t = p2m_ram_ro;
  }

be creative :-)

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