Re: [Xen-devel] [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.02.19 at 00:30, <sstabellini@xxxxxxxxxx> wrote:
> On Wed, 13 Feb 2019, Jan Beulich wrote:
>> >>> On 13.02.19 at 02:17, <sstabellini@xxxxxxxxxx> wrote:
>> > On Tue, 12 Feb 2019, Jan Beulich wrote:
>> >> >>> On 12.02.19 at 13:01, <ian.jackson@xxxxxxxxxx> wrote:
>> >> > I would particularly welcome the opinion of hypervisor maintainers on
>> >> > my type safety point, below.
>> >> 
>> >> I agree with the requirements you put forward; I think I'd
>> >> prefer the inline function versions I had suggested (or
>> >> something similar) over macros though, not the least because
>> >> they come with "built-in" type safety, rather than grafted one
>> >> (by adding "pseudo" comparisons).
>> > 
>> > I don't mind the type checks in principle, I didn't add them to this
>> > version because, as I wrote in a previous email, with have occurrences
>> > of all three this possible calls:
>> > 
>> >   SYMBOL_COMPARE/SUBTRACT( symbol, symbol )
>> >   SYMBOL_COMPARE/SUBTRACT( symbol, non-symbol )
>> >   SYMBOL_COMPARE/SUBTRACT( non-symbol, symbol )
>> > 
>> > If you look through the patches you should be able to spot all three.
>> > As you know "non-symbol" and "symbol" are of different type:
>> > 
>> >   non-symbol would be like a "struct wombat*"
>> >   symbol would be like a "struct wombat[]"
>> 
>> These are declared types. Effective type when used as rvalue
>> (i.e. also when passed as arguments to functions) is struct
>> wombat * in both cases.
> 
> I see..
> 
> 
>> The important aspect (and new idea) Ian has been introducing
>> really is that the "end" symbols intentionally no longer be of
>> the same type as the start ones (but some type derived
>> thereof).
> 
> I missed that part of his proposal. Reading back your suggested static
> inline functions and WHATEVER macro I can see that it works. But I don't
> understand why is it desirable to have the "end" symbols intentionally
> no longer be of the same type as the start ones.

As said - this is to make it impossible to mistakenly invert start and
end in a function (or macro) invocation (without the compiler pointing
out the issue).

> Also, I would ask to consinder the impact of WHATEVER on the code versus
> the var.S approach, which at least has the benefit of being simpler.

I'm not sure of this.

>> >> Furthermore - do we really need both a subtract and a compare
>> >> construct? The result subtract produces can be used for
>> >> comparison purposes as well, after all (just like all CPUs I know
>> >> details of implement [integer] compare as a subtract discarding
>> >> its numeric result, instead [or only] updating certain status flags).
>> > 
>> > No, we don't. In my first attempt I only had one macro. I am happy to
>> > follow your suggestion and keep only SUBTRACT.
>> 
>> Except that, as I think Ian has also suggested, DIFFERENCE() (or
>> SYMBOL_DIFFERENCE()) might be better, as it (hopefully)
>> reduces the connections to the - operator, and hence the risk
>> of possibly getting the argument order wrong. Otoh with the
>> type safety added wrong argument order would cause a
>> compile time error.
> 
> I don't mind either way. The good thing about the way is done in this
> series is that all the comparison signs (<, >, <=, >=, etc) didn't have
> to be changed. It makes it much easier to review and check it's correct.

Yeah, I've realized this as well meanwhile: With

#define DIFFERENCE(s, e) ((e) - (s))

(shortened to just show the essential aspect here) at least one of

    if ( DIFFERENCE(start, end) > 0 )

and

   diff = DIFFERENCE(start, end);

becomes confusing to read. Specifying the arguments the other
way around seems counterintuitive, and swapping the operands of
- would improve the comparison use, but the difference calculation
would then require an un-obvious explicit negation of the result.

Ian, do you have any further thoughts here?

Jan



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