On 06/06/2017 10:57 AM, David Hildenbrand wrote:
>> Let me add one meta comment too. IMHO what makes the function hard to
>> understand, is that we have a bunch of optional parameters, and that
>> although this seems to be some sort of approximation problem (find the
>> best good enough), the metric ain't obvious for somebody not to familiar
>> with the problem domain (like me). AFAIU you are fixing the following
>> problem domain (me). Seems that the metric implemented was sub-optimal:
>> if parameters present {type}, absent {ec_ga} maybe present {gen,
>> features}, then type is not honored appropriately. One can probably
>> figure out what's intended by understanding all the usages (which makes
>> reviewing it demanding). For instance, you also refer to a particular
>> usage in your title/subject (that is "model->def =
>> s390_find_cpu_def(cpu_type, ibc_gen(unblocked_ibc),
>> ibc_ec_ga(unblocked_ibc), NULL);" in target/s390x/kvm.c).
> Actually, I thought that all relevant z/VM systems would properly
> forward the IBC. By relevant, I am speaking about zEC12+. So I never saw
> this while working on the CPU model.
>
> We could split this function into multiple: find by (cpuid),
> (cpuid,gen,ec_ga), (features), (gen,ec_ga,features). All without
> optional parameters. But I doubt that this will result in better code,
> especially on the caller side.
>
> If you have an idea how to improve that piece of code, feel free to send
> a cleanup.
I prefer leaving it as-is. My understanding ain't complete enough anyways.
Making the caller side needlessly complicated is definitely not what we
want. One approach to make understanding/reviewing the function easier
(especially in isolation) would be adding a comment describing it's
pre- and postcondition (akin to Hoare logic but in natural language).
If that can be one easily, then I do not think we have a problem.
My problem is that I can't (that's what I mean by the 'metric'
is not obvious).
>
> Thanks for having a look!
>
yw
Halil
