RFR 8172443: Change use of tree.pos to line:col in rawDiagnostics

[Vicente Romero]

On 10/05/2017 10:40 AM, Maurizio Cimadamore wrote:
>
>
> On 05/10/17 15:27, Vicente Romero wrote:
>>
>>
>> On 10/05/2017 10:12 AM, Maurizio Cimadamore wrote:
>>>
>>>
>>> On 05/10/17 15:05, Vicente Romero wrote:
>>>>
>>>>
>>>> On 10/05/2017 09:51 AM, Maurizio Cimadamore wrote:
>>>>>
>>>>>
>>>>> On 05/10/17 14:49, Vicente Romero wrote:
>>>>>> Hi,
>>>>>>
>>>>>> +100 to this effort, that will save us a lot of time. Regarding 
>>>>>> the patch, sometimes new lines need to be added to a test, 
>>>>>> sometimes in the jtreg header, sometimes somewhere else. How bad 
>>>>>> would be to use the column number only instead of line:col?
>>>>> I understand the concern. I think the goal of this patch is to fix 
>>>>> the #1 offender. As you say there could be other offenders. But 
>>>>> getting rid of them all is hard - if you just use 'col' you 
>>>>> basically lose uniqueness, which makes looking at the golden file 
>>>>> more obscure, I think.
>>>>
>>>> well it's true that uniqueness could be a problem for errors 
>>>> reported at statements that spans more than one line, but I'm not 
>>>> sure if it is worthy to still keep a source of brittleness because 
>>>> of that. I mean what could be the probability of introducing a 
>>>> change that will swap two identical column positions in the 
>>>> diagnostics and still produce the same diagnostics such that we 
>>>> won't detect it. In the worst case we can add the line if we see 
>>>> that the particular statement we are generating the diagnostics for 
>>>> spans more than one line and use only the column for all other cases
>>> I suggest with stick with the proposed approach and then see how bad 
>>> it is in the real world?
>>
>> I think that the proposed approach will be better than the current 
>> situation, that's for sure, but I think that this is the right moment 
>> to consider if the proposed approach can be still better. If you want 
>> to go for it with the current version, I'm OK with that but you know 
>> how long it takes for stuff like this to be revisited.
> I believe the ultimate fix for these kind of issues would be to 
> revisit the code so that instead of generating line or column info, we 
> generate a position that is relative to the start of the enclosing 
> statement. In a way this generalizes over the 'using only col' idea, 
> and works even for functional expressions scattered across multiple 
> lines (but part of the same statement).
>
> That said, doing that would not only require changes to the raw 
> diagnostic backend (as done here), but would need deeper changes in 
> how functional expression trees are communicated to the diagnostic 
> backend. Right now we just pass trees as diagnostic arguments. Trees 
> do not provide any contextual info (e.g. what is the parent of the 
> tree?) - so to do this kind of treatment (and also the treatment you 
> suggest where we only use column 'if needed') requires a contextual 
> info that the current diag arguments do not have.
>
> While this is all solvable, as with all fixes/enhancements, it's about 
> trying to achieve some kind of compromise between complexity and 
> benefits. Having fixed many source pos related issues, I can say with 
> confidence that this patch addresses almost all of them.
>
> When line number changes in a test file (e.g. because more stuff is 
> added), your golden file will have to change anyway, as all the errors 
> generated _after_ the insertion are going to have stale info in the 
> golden file. That's why I did not prioritize the 'vertical' 
> adjustments so much (as they are typically breaking anyway - modulo 
> few lucky cases).

yes this is a real concern and will invalidate the golden file anyway as 
the line numbers generated there are absolute, and I think it is enough 
discussion for this small feature, so go for it.

>
> Maurizio

Vicente

>>
>>>
>>> Maurizio
>>
>> Vicente
>>
>>>>
>>>> Vicente
>>>>
>>>>>
>>>>> Maurizio
>>>>>>
>>>>>> Thanks,
>>>>>> Vicente
>>>>>>
>>>>>> On 10/05/2017 09:29 AM, Maurizio Cimadamore wrote:
>>>>>>> Hi,
>>>>>>> this is a fix to our raw diagnostic machinery that will allow 
>>>>>>> for more robust compiler negative tests. Currently, when a 
>>>>>>> functional expression is found in a diagnostic, it can sometimes 
>>>>>>> be represented using the absoluted cursor position of the 
>>>>>>> expression in the source file. This makes the golden files for 
>>>>>>> such tests extremely unreliable, as simply adding/removing chars 
>>>>>>> from lines before the functional expression would result in 
>>>>>>> golden file mismatches. This is extremely annoying when e.g. the 
>>>>>>> jtreg header of a test must be change for some reason (e.g. to 
>>>>>>> add an extra bug id).
>>>>>>>
>>>>>>> The solution is to use a more robust encoding with line:col - so 
>>>>>>> that there's less dependencies on what happens on previous lines.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~mcimadamore/8172443/
>>>>>>>
>>>>>>> Cheers
>>>>>>> Maurizio
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>