12

I recently ran across a routine that looks something like this:

procedure TMyForm.DoSomething(list: TList<TMyObject>; const flag: boolean); var local: integer; begin if flag then //do something else local := ExpensiveFunctionCallThatCalculatesSomething; //do something else for i := 0 to list.Count do if flag then //do something else if list[i].IntValue > local then //WARNING HERE //do something else end;

This gives Variable 'local' might not have been initialized even though you can tell by reading the code that you won't hit that line unless the code branch that initializes it has run.

Now, I could get rid of this warning by adding a useless local := 0; at the top of the procedure, but I wonder if there might not be a better way to structure this to avoid the issue. Anyone have any ideas?

Improve this question
9
  • I thought pre-initializing variables was good practise considering they otherwise point to random data in memory and could lead to unexpected behavior in your application if you don't pay attention to it :) C# I think wont even allow you to run that code because local isn't initialized. Commented Apr 26, 2010 at 22:35
  • Oh, I know it's a good practice. I just wonder if there isn't a better way to write the code where I don't need to initialize it in order to not get that warning. (And turning off warnings is cheating.) Commented Apr 26, 2010 at 22:38
  • I guess the compiler just isn't that sophisticated. It probably only goes as far as seeing that local might be initialized. Commented Apr 26, 2010 at 22:39
  • 1
    "even though you can tell by reading the code that you won't hit that line unless the code branch that initializes it has run" -- The compiler is smart, but not that smart. It's a subtle thing you are doing to make sure that local is never accessed if it isn't initialized, and it errs on the side of the warning. Commented Apr 27, 2010 at 1:00
  • 4
    Somebody really should also point out the index over-run on the loop counter. ;) Commented Apr 27, 2010 at 2:19

5 Answers 5

Reset to default
12

I would separate it into two for-loops--one for when flag is true and one for when flag is false. As an added benefit, you won't have to execute the if-statement on every iteration.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

Highly unlikely I would say, but you could of course code up the routine in both ways and then compare the CPU disassembly view of the code in each case to determine for yourself. No need to :wonder: ;)
Yeah, that's a decent performance optimization, but I don't see how it would eliminate the compiler warning.
Because in the "if NOT flag" branch the variable would be initialised before being used, and in the alternate flow it would not be used (or referenced) at all.
@Mason, it should eliminate the warning, because in the flag=true branch, you never try to access the value of local. Try it.
6

IMO, the assignment to 0 isn't useless here - it's beneficial to maintanability. So you'll save someone (perhaps your future self) from having to spend a minute or two to determine that the code works. And the design cleverness will likely be lost on them (even if it's you!)

Improve this answer

2 Comments

Sometimes this assignment is harmful because it shuts up the compiler but that just hides a semantic error: Say Mason's successor mistakenly replaces the if condition by if not flag then. Then the warning would be justified but the local := 0; would suppress it. So in this case I would consider it not even useless but a pessimization.
I've seen cases where adding initialization gets rid of the bogus might not have been initialized warning, only to produce another warning that value assigned to x is never used - seems the compiler sometimes just can't put two and two together.
6

Refactor the code to contain two separate flows based on the flag parameter:

procedure TMyForm.DoSomething(list: TList<TMyObject>; const flag: boolean); var local: integer; begin if flag then begin //do something //do something else for i := 0 to Pred(list.Count) do //do something end else begin local := ExpensiveFunctionCallThatCalculatesSomething; //do something else for i := 0 to Pred(list.Count) do if list[i].IntValue > local then //do something else end; end;

This essentially restates the answer given by neilwhitaker1, but also makes it clear that the initialisation of the local variable is to be brought inside the conditional branch, which is what addresses the compiler warning (which is only emitted if the varialbe used in a branch where it may not be initialised - in the branch that does not use it at all no such warning shall be emitted, and in the branch where it is used it is certain to be initialised, and since it is used in one branch you will not get a "may not be used" hint either.

NOTE: If any of the "// something else"'s are common to each branch, these could of course be refactored as local, nested procedures to avoid duplication.

ALSO NOTE: In the code above I have corrected the loop index over-run on the for loop. :)

Improve this answer

2 Comments

Added remark : the two branches look different enough. Maybe split it in two functions : "if flag then DoSomethingSimple else DoSomethingExpensive;"
That is a further refactoring decision for the original author - it's impossible to say just how similar the two branches are without knowing what the "something" and "something elses's" are. :)
1

adding local:=0 good solution.

as i know the hints because can be uninitialized variables. if you always assign variable values for initing and using try finally blocks for error checking you can't live any problems.

as i know compiler give hints because even if you check flag at runtime your variable can be unassigned and your code can't run as expected.

sorry for my bad english :)

Improve this answer

Comments

1

i would change the top of your procedure to

procedure TMyForm.DoSomething(list: TList<TMyObject>; const flag: boolean); var local: integer; begin if flag then begin local := 0; //do something end else local := ExpensiveFunctionCallThatCalculatesSomething;

etc...

That way Local is set no matter what flag is, more so it's not being set twice if flag is false

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.