Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 23, 2018 at 11:40:34AM +0000, Andrew Cooper wrote:
> On 22/02/18 13:39, Jan Beulich wrote:
> >>>> On 22.02.18 at 13:39, <george.dunlap@xxxxxxxxxx> wrote:
> >> On 02/22/2018 12:22 PM, Jan Beulich wrote:
> >>>>>> On 22.02.18 at 12:41, <andrew.cooper3@xxxxxxxxxx> wrote:
> >>>> On 22/02/18 11:33, Jan Beulich wrote:
> >>>>>>>> On 22.02.18 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> >>>>>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, 
> >>>>>> in line
> >>>>>> with cc-option-add.  Update all callers.
> >>>>> I'm not convinced - cc-option-add makes relatively clear that
> >>>>> something is being added to the options passed to CC. If I
> >>>>> take as-insn-add this way, the macro would need to add an
> >>>>> insn to the AS invocation. While I agree as-insn-check doesn't
> >>>>> make clear that it adds any options, I still find this less
> >>>>> misleading than the suggested new name. Let's see what
> >>>>> others think.
> >>>> I'm open to better name suggestions.
> >>> The best I can come up with is, well, as-insn-check, as that
> >>> reasonably describes at least part of what the construct does.
> >>> as-insn-check-and-add-option, besides being too long, isn't
> >>> meaningfully better.
> >> We're definitely getting into bikeshed territory here.
> > Indeed, but I think a change in name should be an improvement,
> > not going from one questionable name to another questionable
> > one.
> >
> >>  I agree with
> >> Andy that 'check' doesn't really convey that something changed.  Is the
> >> check-and-add "add it if it doesn't exist already"?  Or add it if some
> >> other check passes / fails?
> > It is "check if this piece of assembly assembles and add the
> > provided option to the indicated variable", extended by Roger's
> > patch to "..., and add the other provided option if it doesn't
> > assemble".
> 
> Ok - how do we unblock this?
> 
> There appears to be agreement that as-insn-check isn't a great name, and
> my proposed as-insn-add isn't much better.
> 
> The base runes of as-insn and cc-option are compatible.  They check the
> fragment, and yield one of two options.  cc-option-add and as-insn-check
> are built on top of the base runes, and mutate the flags passed in.
> 
> as-check-frag-update-option ?

That seems overly long, and TBH I think almost everyone not familiar
with the code would have to go an look what this macro does.

I'm fine with either as-insn-check, as-insn-add or as-option-add, but
I would also like to unblock this. I guess if there's no consensus we
just leave the current one?

Roger.

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