WTF? Using the function Result as the for loop variable?
WTF? Using the function Result as the for loop variable?
function TGrepHistoryList.SearchHistoryItem(AGrepSettings: TGrepSettings; var AHistoryItem: TGrepHistoryListItem): Integer;
begin
AHistoryItem := nil;
for Result := 0 to HistoryList.Count - 1 do
begin
AHistoryItem := HistoryItems[Result];
if not FEnabled and (Result = 0) then
Break;
if AnsiSameText(AHistoryItem.SearchText, AGrepSettings.Pattern) then
Break;
AHistoryItem := nil;
end;
end;
It compiles and seems to work, but considering that similar code ...
for i := 0 to bla.count-1 do begin
// do something
end;
WriteLn(i)
.. generates the warning that a for loop variable might not be assigned after the loop, this is really odd.
EDIT:
Now, this is curious:
function TGrepHistoryList.SearchHistoryItem(AGrepSettings: TGrepSettings; var AHistoryItem: TGrepHistoryListItem): Integer;
begin
AHistoryItem := nil;
for Result := 0 to HistoryList.Count - 1 do
begin
AHistoryItem := HistoryItems[Result];
if not FEnabled and (Result = 0) then
Exit;
if AnsiSameText(AHistoryItem.SearchText, AGrepSettings.Pattern) then
Exit;
AHistoryItem := nil;
end;
end;
If I replace Break with Exit (as in the example from David Heffernan
), I get the warning "W1037: FOR-Loop variable "Result" might be undefined after loop.
function TGrepHistoryList.SearchHistoryItem(AGrepSettings: TGrepSettings; var AHistoryItem: TGrepHistoryListItem): Integer;
begin
AHistoryItem := nil;
for Result := 0 to HistoryList.Count - 1 do
begin
AHistoryItem := HistoryItems[Result];
if not FEnabled and (Result = 0) then
Break;
if AnsiSameText(AHistoryItem.SearchText, AGrepSettings.Pattern) then
Break;
AHistoryItem := nil;
end;
end;
It compiles and seems to work, but considering that similar code ...
for i := 0 to bla.count-1 do begin
// do something
end;
WriteLn(i)
.. generates the warning that a for loop variable might not be assigned after the loop, this is really odd.
EDIT:
Now, this is curious:
function TGrepHistoryList.SearchHistoryItem(AGrepSettings: TGrepSettings; var AHistoryItem: TGrepHistoryListItem): Integer;
begin
AHistoryItem := nil;
for Result := 0 to HistoryList.Count - 1 do
begin
AHistoryItem := HistoryItems[Result];
if not FEnabled and (Result = 0) then
Exit;
if AnsiSameText(AHistoryItem.SearchText, AGrepSettings.Pattern) then
Exit;
AHistoryItem := nil;
end;
end;
If I replace Break with Exit (as in the example from David Heffernan
), I get the warning "W1037: FOR-Loop variable "Result" might be undefined after loop.
That's a fairly common pattern, you see it quite a bit in the RTL/VCL. You are correct that the compiler should warn. That is does not is criminal. Typically I would use this pattern like so:
ReplyDeletefunction Find(const Value: T): Integer;
begin
for Result := 0 to Count-1 do begin
if Items[Result] = Value then begin
exit;
end;
end;
Result := -1;
end;
Or if you prefer to run the gauntlet and use single statements:
function Find(const Value: T): Integer;
begin
for Result := 0 to Count-1 do
if Items[Result] = Value then
exit;
Result := -1;
end;
Setting Result after the loop terminates, or raising an exception, is, for me, non-negotiable.
The code you present presumably follows a contract that allows the returned index to be undefined if the returned item is nil. I regard that as dubious. Were it me I'd have the code return -1 when then returned item is nil. But all of that is completely unrelated to the use of Result as loop variable.
So, that said, I would write your function like so:
function TGrepHistoryList.SearchHistoryItem(AGrepSettings: TGrepSettings; var AHistoryItem: TGrepHistoryListItem): Integer;
begin
for Result := 0 to HistoryList.Count - 1 do
begin
AHistoryItem := HistoryItems[Result];
if not FEnabled and (Result = 0) then
exit;
if AnsiSameText(AHistoryItem.SearchText, AGrepSettings.Pattern) then
exit;
end;
Result := -1;
AHistoryItem := nil;
end;
or (I prefer this one) like this
function TGrepHistoryList.SearchHistoryItem(AGrepSettings: TGrepSettings; var AHistoryItem: TGrepHistoryListItem): Integer;
begin
if not FEnabled and (HistoryList.Count > 0) then
begin
Result := 0;
AHistoryItem := HistoryItems[Result];
exit;
end;
for Result := 0 to HistoryList.Count - 1 do
begin
AHistoryItem := HistoryItems[Result];
if AnsiSameText(AHistoryItem.SearchText, AGrepSettings.Pattern) then
exit;
end;
Result := -1;
AHistoryItem := nil;
end;
The loop variable not being assigned warning is related to internal compiler optimizations (usually it's the internal loop counter reversal) more than any higher level logic. I do not know if in that case the optimization is disabled (and thus the warning is unnecessary) or if it's still active, and the lack of warning is an oversight...
ReplyDeleteIt's documented http://docwiki.embarcadero.com/RADStudio/en/W1037_FOR-Loop_variable_'%25s'_may_be_undefined_after_loop_(Delphi)
ReplyDeleteAgustin Ortu thanks for the link.
ReplyDeletebtw: that example code is really bad. Due to the wrong indentation it suggests that the line with the warning is part of the for loop. Yes, that happens in real code, but no, it's not a good example to show the point of the warning.
Thomas Mueller There should be a line
ReplyDeleteResult := -1;
at the end of the function, just in case if no match was found.
Even if this is valid code, and the Delphi compiler allows this, I don't like such constructs (it is so C-ish ;-) ) . I always spend a local loop variable.
From what i remember, in Delphi it makes a difference whether you use the loop variable or not. If you don't, you won't even be able to use it in a watch to see its value while you're debugging. I guess it generates machine code that counts back to 0 so that it's faster to check if a for loop finished.
ReplyDeleteIn any case, because the exact behavior seems to be compiler- and situation-specific, I'd avoid relying on a loop variable's value anywhere outside the scope of the loop itself. It's not that hard to rewrite this code in a more conventional way.
So, yes, even when this compiles and works, it still sounds like a bad idea to me to do this.
Counting back to zero? Only if loop var is not referred to in loop body.
ReplyDeleteAchim Kalwa No, adding Result := -1 would break the code. There are no Exits but Breaks.
ReplyDeleteOh, I didn't look into the real code, but at your 2nd example, which uses exit.
ReplyDeleteThis is perfectly valid code. When the loop variable is used inside the loop there is no "counting down to zero" optimization applied. And exiting from within the loop is preventing any post loop usage of the loop variable thus using Result as loop variable is legit.
ReplyDeleteAchim Kalwa is right Thomas Mueller, you get the warning in your second example (with the Exits), because you don't assign anything to result after the loop.
ReplyDeleteStefan Glienke: it's valid code, no doubt. It's still not a good idea if you'd ask me. It's really easy to end up with a situation like this:
ReplyDeletefunction GetNum:integer;
begin
for Result := 0 to 9 do
begin
Write('.');
if Result>100 then
break;
end;
end;
Note that the loop never breaks.
The compiler won't complain, but you're really using the loop variable outside of the look like this.
So, this function returns "10", which is basically a nonsense number.
Also, if the number of iterations turns out to be 0 (or negative), Result remains uninitialized, and then all bets are off about what you'll get in return.
There's a difference beween "valid code" and "a good idea" :-)
Wouter van Nifterick No, the returned value is not 10, it is undefined.
ReplyDeleteStefan is imagining code like that in my first comment I expect. Code where the return value is defined.
David Heffernan my point is that this is a dangerous coding style.
ReplyDeleteIf HistoryList.Count is 0, "Result" remains uninitialized, and you'll get whatever was found on the memory address where Result would be placed.
If the pattern is not found, the loop loop runs uninterrupted without the break, and you'll get something called "undefined". (in practice, you'll get a value equal to HistoryList.Count in this code).
But yes, the compiler doesn't complain, which officially makes it "valid code". And apparently the code "works". Still, the code can introduce bugs that might be hard to trace, because the outcome can be unpredictable in some situations.
Wouter van Nifterick Read the code in my first comment please
ReplyDeleteDavid Heffernan: yes your code is safe because you ensure that the result gets set when the loops doesn't run, and that the value is defined when the loop finishes uninterupted.
ReplyDeleteNow i get why you're in defense mode. I was commenting on the original code, not yours.
Wouter van Nifterick The code in the original post is bogus. My comment is to point out that using result variable as loop variable is not inherently wrong.
ReplyDeleteWhether or not a variable is well defined is not determined by whether it is a local or the result variable.