shineworld 73 Posted June 15 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 ? 1 Share this post Link to post
dummzeuch 1456 Posted June 15 (edited) 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 Share this post Link to post
shineworld 73 Posted June 15 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. Share this post Link to post
dummzeuch 1456 Posted June 15 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. Share this post Link to post
Uwe Raabe 1979 Posted June 15 (edited) 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 14 Share this post Link to post
dummzeuch 1456 Posted June 15 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. 😞 2 Share this post Link to post
Uwe Raabe 1979 Posted June 15 1 hour ago, dummzeuch said: Which again highlights the value of a code formatter. 👍 Share this post Link to post
Rollo62 520 Posted June 16 (edited) 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 2 Share this post Link to post
Die Holländer 33 Posted June 17 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.. 1 Share this post Link to post
Uwe Raabe 1979 Posted June 17 Given the use of Data in the following lines, I suspect that the begin end should wrap a much larger region. Share this post Link to post
David Schwartz 421 Posted June 17 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. Share this post Link to post
JonRobertson 63 Posted June 17 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: 2 Share this post Link to post
shineworld 73 Posted June 18 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"... Share this post Link to post
Lajos Juhász 270 Posted June 18 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. Share this post Link to post
shineworld 73 Posted June 18 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..... Share this post Link to post
haentschman 92 Posted June 18 Quote I've used WinMerge to copy some line in the past, ...VCS? (VersionControlSystem) Quote copy some line in the past ...from ZIP? 🤢 Share this post Link to post
Cristian Peța 100 Posted June 18 3 minutes ago, shineworld said: The pas file was corrupted This should be easily spotted with a source control. Share this post Link to post
shineworld 73 Posted June 18 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. Share this post Link to post
dummzeuch 1456 Posted June 18 (edited) 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 Share this post Link to post