Variable might not have been initialized

[shineworld 73]

I have a strange warning in compilation with Sydney 10.4.1 that I just can't understand:

 

procedure TWorkOrderListFrame.WorkOrderFilesGridDrawCell(Sender: TObject; ACol, ARow: Integer; Rect: TRect; State: TGridDrawState);
var
  R: TRect;
  C: TColor;
  Value: string;
  Canvas: TCanvas;
  PenColor: TColor;
  BrushColor: TColor;
  FileIndex: Integer;
  Data: TWorkOrderData;
  BackgroundColor: TColor;
  WorkOrdersSelectedRow: Integer;
begin
  // avoids any change at header row
  if ARow = 0 then Exit;

  // checks and gets work order data
  WorkOrdersSelectedRow := WorkOrdersGrid.Selection.Top;
  if FWorkOrderManager.GetWorkOrderDataByIndex(WorkOrdersSelectedRow - 1, Data) then

  // evaluates file index
  FileIndex := ARow - 1;
  if (FileIndex < 0) or (FileIndex >= TWorkOrderData.FILE_DATA_SIZE) then Exit;
  if Data.FileData[FileIndex].FileName = '' then Exit;

  // recovers canvas
  Canvas := WorkOrderFilesGrid.Canvas;

[dcc64 Warning] osWorkOrderListFrame.pas(946): W1036 Variable 'FileIndex' might not have been initialized

The line with the warning is:

if (FileIndex < 0) or (FileIndex >= TWorkOrderData.FILE_DATA_SIZE) then Exit;

but FileIndex is set just the line above....

Any idea ?

[dummzeuch 1436]

Looks like a compiler bug (but isn't, see Uwe's Answer!). I've seen similar warnings / hints. It's even worse when you use different Delphi versions: One complains about a variable not being initialized, so you fix that by adding a superfluous assignment, then the other complains about a value assigned to that variable not being used.

The annoying bit is that I usually want my code to compile without hints and warnings so I can spot new problems right away.

Edited June 15 by dummzeuch

[shineworld 73]

I feel exactly the same way about Hint and Warnings, usually I've got 0/0.
They are an indispensable tool to guarantee good code, especially when we are talking about millions of lines in play.

At the moment I am compiling this project still with Sydney, although I also have Athens on which I have yet to test many

third-party libraries that require more up-to-date versions and generate too many Warnings.

[dummzeuch 1436]

I usually solve this kind of warnings with a compiler specific ifdef that adds an assignment for the variable in question. In this case I'm not sure this is possible as there already is an unconditional assignment.

{$IF CompilerVersion=nn}
// Delphi xxx complains here about FileIndex not being initialized
FileIndex := 0;
{$IFEND}
FileIndex := ARow - 1;

I make it compiler version specific just in case a newer compiler does no longer complain. The downside is that it clutters the code with such workarounds, but I look at a specific code section a lot less than I see unnecessary hints and warnings.

[Uwe Raabe 1960]

Reading the code despite of its irritating formatting reveals the problem:

  WorkOrdersSelectedRow := WorkOrdersGrid.Selection.Top;
  if FWorkOrderManager.GetWorkOrderDataByIndex(WorkOrdersSelectedRow - 1, Data) then
    // evaluates file index
    FileIndex := ARow - 1;

  if (FileIndex < 0) or (FileIndex >= TWorkOrderData.FILE_DATA_SIZE) then
    Exit;

FileIndex will not be initialized when the if clause fails.

 

BTW, it also irritates that the compiler is charged guilty first.

Edited June 15 by Uwe Raabe

[dummzeuch 1436]

1 hour ago, Uwe Raabe said:

BTW, it also irritates that the compiler is charged guilty first.

Which again highlights the value of a code formatter. I'm so used to that that I overlooked the if statement. 😞

 

[Uwe Raabe 1960]

1 hour ago, dummzeuch said:

Which again highlights the value of a code formatter.

👍

[Rollo62 507]

Yees, thats a nasty bug.
Thats why I had choosen long time ago, to always add a proper scope around even the simplest lines.
 

if FWorkOrderManager.GetWorkOrderDataByIndex(WorkOrdersSelectedRow - 1, Data) then
begin
  // evaluates file index
  FileIndex := ARow - 1;
end;

IMHO that increases readability and avoid a lot of such hard to see errors.

You're lucky here, that the compiler throwed a warning 🙂

 

Edited June 17 by Rollo62

[Die Holländer 32]

19 hours ago, Rollo62 said:

Yees, thats a nasty bug.
Thats why I had choosen long time ago, to always add a proper scope around even the simplest lines.

 

Like DelphiLint (shameless promotion..) advise you to do..

 

[Uwe Raabe 1960]

Given the use of Data in the following lines, I suspect that the begin end should wrap a much larger region.

[David Schwartz 417]

On 6/15/2024 at 2:08 AM, Uwe Raabe said:

Reading the code despite of its irritating formatting reveals the problem:

  WorkOrdersSelectedRow := WorkOrdersGrid.Selection.Top;
  if FWorkOrderManager.GetWorkOrderDataByIndex(WorkOrdersSelectedRow - 1, Data) then
    // evaluates file index
    FileIndex := ARow - 1;

  if (FileIndex < 0) or (FileIndex >= TWorkOrderData.FILE_DATA_SIZE) then
    Exit;

FileIndex will not be initialized when the if clause fails.

 

BTW, it also irritates that the compiler is charged guilty first.

Look for the code that causes FileIndex to be set, and then look above it for something that might cause that line to not execute. 

 

You're lucky in this case that these are three lines in a row. Sometimes several of these kinds of initializations are set at the top of a proc and the warning isn't issued until much farther down in the body of the proc.

[JonRobertson 50]

On 6/15/2024 at 6:42 AM, Uwe Raabe said:

On 6/15/2024 at 5:20 AM, dummzeuch said:

Which again highlights the value of a code formatter.

👍

I set that compiler warning (and a couple others) as an Error in every project:

 

[shineworld 73]

Sorry but
 

procedure TWorkOrderListFrame.WorkOrderFilesGridDrawCell(Sender: TObject; ACol, ARow: Integer; Rect: TRect; State: TGridDrawState);
var
  R: TRect;
  C: TColor;
  Value: string;
  Canvas: TCanvas;
  PenColor: TColor;
  BrushColor: TColor;
  FileIndex: Integer;
  Data: TWorkOrderData;
  BackgroundColor: TColor;
  WorkOrdersSelectedRow: Integer;
begin
  // avoids any change at header row
  if ARow = 0 then Exit;

  // checks and gets work order data
  WorkOrdersSelectedRow := WorkOrdersGrid.Selection.Top;
  if FWorkOrderManager.GetWorkOrderDataByIndex(WorkOrdersSelectedRow - 1, Data) then

  // evaluates file index
  FileIndex := ARow - 1;  // <--- Here FileIndex is initialized with ARow (in update cell grid ROW) - 1 because first row is fixed row.
  if (FileIndex < 0) or (FileIndex >= TWorkOrderData.FILE_DATA_SIZE) then Exit; // <--- here the FileIndex is checked to be inner 0 to TWorkOrderData.FILE_DATA_SIZE - 1) otherwise exit (none to do).
  if Data.FileData[FileIndex].FileName = '' then Exit;

  // recovers canvas
  Canvas := WorkOrderFilesGrid.Canvas;

Sincerely FileIndex, at if (FileIndex < 0.... ) it already "defined" but compling phase say "undefined"...

[Lajos Juhász 263]

5 minutes ago, shineworld said:

Sincerely FileIndex, at if (FileIndex < 0.... ) it already "defined" but compling phase say "undefined"...

We already know that it is not defined in case when 

FWorkOrderManager.GetWorkOrderDataByIndex(WorkOrdersSelectedRow - 1, Data)

 

returns false.

[shineworld 73]

ohhh I've checked... The pas file was corrupted, I've used WinMerge to copy some line in the past, and IDE shown text "then Exit" but compiler and copy/paste does not....

Fixed replacing all code and now works.
So sorry.....

[haentschman 92]

Quote

I've used WinMerge to copy some line in the past,

...VCS? (VersionControlSystem)

Quote

copy some line in the past

...from ZIP? 🤢

 

[Cristian Peța 96]

3 minutes ago, shineworld said:

The pas file was corrupted

This should be easily spotted with a source control.

[shineworld 73]

Normally I use only an internal GIT server (gitea).
But this part of the source code was copied with WinMerge from a portable SSD  which gave me a lot of issues and I've dismissed it recently.

Thank you to all for support.

[dummzeuch 1436]

27 minutes ago, shineworld said:

Sorry but
 

procedure TWorkOrderListFrame.WorkOrderFilesGridDrawCell(Sender: TObject; ACol, ARow: Integer; Rect: TRect; State: TGridDrawState);
var
  R: TRect;
  C: TColor;
  Value: string;
  Canvas: TCanvas;
  PenColor: TColor;
  BrushColor: TColor;
  FileIndex: Integer;
  Data: TWorkOrderData;
  BackgroundColor: TColor;
  WorkOrdersSelectedRow: Integer;
begin
  // avoids any change at header row
  if ARow = 0 then Exit;

  // checks and gets work order data
  WorkOrdersSelectedRow := WorkOrdersGrid.Selection.Top;
  if FWorkOrderManager.GetWorkOrderDataByIndex(WorkOrdersSelectedRow - 1, Data) then

  // evaluates file index
  FileIndex := ARow - 1;  // <--- Here FileIndex is initialized with ARow (in update cell grid ROW) - 1 because first row is fixed row.
  if (FileIndex < 0) or (FileIndex >= TWorkOrderData.FILE_DATA_SIZE) then Exit; // <--- here the FileIndex is checked to be inner 0 to TWorkOrderData.FILE_DATA_SIZE - 1) otherwise exit (none to do).
  if Data.FileData[FileIndex].FileName = '' then Exit;

  // recovers canvas
  Canvas := WorkOrderFilesGrid.Canvas;

Sincerely FileIndex, at if (FileIndex < 0.... ) it already "defined" but compling phase say "undefined"...

No. That line is only executed if the condition in the if statement three lines above it is true. Let me adapt the code to make this clear:

  if FWorkOrderManager.GetWorkOrderDataByIndex(WorkOrdersSelectedRow - 1, Data) then begin
    // evaluates file index
    FileIndex := ARow - 1;  // <--- Here FileIndex is initialized, but only if the condition above was true
  end;
  if (FileIndex < 0) or (FileIndex >= TWorkOrderData.FILE_DATA_SIZE) then Exit; // <--- here FileIndex mith not have been initialized if the condition above was false. And that's what the compiler says.

 

Edited June 18 by dummzeuch