Page 2 of 2
Re: #7 → #16 TRAP in DevAnalyzer.UseObj
Posted: Sat Oct 04, 2014 11:10 am
by DGDanforth
Good,
The solution of #16 looks good.
I'll start a vote tomorrow.
-Doug
Re: #7 → #16 TRAP in DevAnalyzer.UseObj
Posted: Mon Oct 06, 2014 12:46 am
by DGDanforth
Chris wrote
In order to cast an informed vote I would like to see a precise definition of the bug - just a single code example that fails does not adequately document the nature of the problem. As a minimum, something on the lines of (assuming my understanding is correct):
DevAnalyzer in Blackbox 1.6 Final identifies when an index variable is modified within a FOR loop.
However in a very specific rare case it results in a trap:
1. There is a nested FOR loop
AND
2. The same index variable is used for each FOR loop
AND
3. The index variable is assigned to another variable within the FOR loop
If the fix solves other instance of traps then they should also be clearly identified. I would also like to see some evidence that regression testing has been done using other relevant code samples to ensure that the fix does not have any unwelcome side effects.
I agree with Chris. This opens a can of worms. For any code fix, how can we know if it introduces other undesired side effects? If the fix is small and "obvious" then we can all agree. But if the fix has any degree of complexity then its consequences will not be obvious (at least to most of us with limited short term memory).
For the DevAnalyzer case one must ask "What is the goal of the analyzer"? Is it to point out invalid "semantics"? Is it to guarantee "correct" execution? What is the "post condition" sought?
Consider
Code: Select all
k := 0; j := 1;
FOR i := 0 TO n-1 DO i := j + k; Out.Int(i); Out.Ln; k := j; j := i; END
Which modifies the loop variable but produces the Fibonacci series. Is that "invalid code"?
To guarantee the loop variable is not modified one would need that variable to be used in a "read only" environment such as
where
Code: Select all
PROCEDURE P(i: INTEGER);
...
END P;
To enforce that one would need to modify the CP syntax to only allow procedures with the correct signature in a FOR loop.
Alternatively, Iterators could be used
Code: Select all
it.First;
WHILE it.Next(i) DO ... END;
Where First and Next are methods of an Iterator type.
But that is up to the programmer.
Since DevAnalyzer only gives warnings (with the fix, that is) its misbehavior is not critical and those other, as yet undetected, cases will rear their heads when encountered.
Re: #7 → #16 TRAP in DevAnalyzer.UseObj
Posted: Mon Oct 06, 2014 3:59 am
by cfbsoftware
DGDanforth wrote:To guarantee the loop variable is not modified one would need that variable to be used in a "read only" environment such as
where
Code: Select all
PROCEDURE P(i: INTEGER);
...
END P;
There may well be a solution which is less restrictive than that. Recently I modified the ARM Oberon compiler used in
Astrobe to prohibit the modification of a FOR loop variable. The solution was simple because Oberon variable identifiers already have an internal read-only property (used for imported items). All I had to do was switch on the read-only property of the index variable while it was in the scope of the FOR loop. It is very likely that a similar technique could be used in Component Pascal as a similar read-only attribute is also available. I think I made use of it some years ago when I was experimenting with extending the usefulness of CP's
IN parameters so they could be used to optionally specify scalar variable parameters (INTEGERS, REALS etc.) as read-only just like you can with structured parameters (ARRAY, RECORD).
Re: #7 → #16 TRAP in DevAnalyzer.UseObj
Posted: Mon Oct 06, 2014 5:46 am
by Ivan Denisov
Changing variable inside loop does not prohibited, so we should not do it read only. The warning during the analysis is enough. The principal thing is that TRAP not appearing anymore.
Re: #7 → #16 TRAP in DevAnalyzer.UseObj
Posted: Mon Oct 06, 2014 6:31 am
by cfbsoftware
Ivan Denisov wrote:Changing variable inside loop does not prohibited, so we should not do it read only.
You are correct in that it is not explicitly prohibited in the Component Pascal Language report or in the Oberon Language report. However, that might just mean that the compiler implementer is not obliged to detect it.
Modifying a FOR lop counter is generally considered to be bad programming practice and is likely to result in obtuse code. The reason that it is detected by the analyser is that it is likely to be a programming error rather than an intentional action. The following directions on the use of the FOR loop appear in Wirth's book 'Programming in Oberon' (2011 Revision):
It is recommended that the for statement be used in simple cases only; in particular, no components of the expressions determining the range must be affected by the repeated statements, and, above all, the control variable itself must not be changed by the repeated statements.
I accept that it may be considered too strict to make it an error in the Blackbox compiler. However, I would strongly advise any programmer considering contravening the above directives to use either a WHILE loop or a REPEAT loop instead.
Re: issue-#16: TRAP in DevAnalyzer.UseObj
Posted: Tue Oct 07, 2014 9:34 am
by Ivan Denisov
According the
voting this issue can be merged to master.
Re: issue-#16: TRAP in DevAnalyzer.UseObj
Posted: Tue Oct 07, 2014 10:12 am
by Josef Templ
I have pulled the changes to master and closed the issue.
I have also removed some of the no longer needed topic branches.
- Josef
Re: issue-#16: TRAP in DevAnalyzer.UseObj
Posted: Tue Oct 07, 2014 10:19 am
by Ivan Denisov
Josef Templ wrote:I have pulled the changes to master and closed the issue.
I have also removed some of the no longer needed topic branches.
Well. I like such working strategy. It works smooth with your help
