[m-rev.] for review: fix half of Mantis bug #548

[Zoltan Somogyi]

2022-02-10 16:13 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
>> I can see four ways to fix the second problem in Mantis 548.
>> Here they are, but this discussion makes sense only if you have read
>> the comments in the new test case descrbing that problem.
>> 
>> 1. Change how we represent HLDS vars in the MLDS. At the moment,
>> that representation, lvn_prog_var, consists of the HLDS variable's name
>> and its number. This means that if we generate code twice for the same
>> HLDS code, we will get the same MLDS name for every HLDS variable
>> in that HLDS code. We could fix this by including in lvn_prog_var
>> a unique integer, allocated from a counter when the MLDS variable
>> is constructed, either in addition to, or in place of, the HLDS variable number.
>> This should be simple to implement, but would make HLDS dumps
>> harder to read, either by adding clutter (if we keep the HLDS variable number)
>> or by removing a link to the HLDS (if we replace the HLDS variable number).
>> 
>> 2. We could keep locals vars as they are, but add the unique integer
>> to the field name that we turn MLDS variables into. This would involve
>> adding an integer, again allocated from a counter, to fvn_env_field_from_local_var.
>> This may be somewhat harder to implement, because it breaks the current
>> direct correspondence between the local var and field name versions
>> of a HLDS variable.
>> 
>> 3. The name collision for field vars occurs because two different switch arms,
>> which are independent pieces of code, use the same environment structure.
>> We could change things so that independent pieces of code get separate
>> environment structures. This change would be so complex that I definitely
>> won't be doing it :-(
>> 
>> 4. As a compromise, when we construct the environment structure,
>> we could replace whatever code appends the lists of local vars from
>> different places to construct the list of fields to put into the environment
>> structure with code that unions sets of local vars instead, so that
>> even if two independent pieces of code specify the same local variable
>> (as in the case of the code that tickled this bug), we won't get duplicate
>> field names. This approach would probably not be too hard to implement,
>> but I definitely wouldn't be able to write a convincing correctness
>> argument in its favor :-(
>> 
>> Opinions?
> 
> 1. Seems overkill to me.
> 
> 2. Sounds okay to me (barring difficulty of implementation).
> 
> 4. It sounds like a variation of (2) which avoids adding distinguishing
> suffixes unless a name collision would arise, which would be a slightly
> better result than (2), but may not be worth the extra complexity.
> But you seem to suggest that (4) would be easier than (2).

In the absence of a correctness argument, I won't do 4.
As for the difficulty levels I listed, they are just rough guesses.

Actually, unless someone objects, I will do a variation of approach 5
I posted about 20 minutes after the email you replied to. This would
fall back to implementing the switch using an if-then-else chain if the code
of an arm that would otherwise have to be shared between two different
ptags contains both (a) code that will cause the construction of
an environment, and (b) some block-local variables that would
then become fields in that environment. Absent either of those factors,
you can't get a duplicated field name (from this source, that is).

> Perhaps --inlining is not necessary given the
> pragma inline?

I forget whether the inlining pass is run in the absence of
the option but in the presence of a pragma, but there is
no point in not specifying the option.

> The rest looks fine.

I followed all your suggestions. Thanks for the review.

Zoltan.