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?

Mason Wheeler
  • 82,511
  • 50
  • 270
  • 477
  • 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. – Jonas B Apr 26 '10 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.) – Mason Wheeler Apr 26 '10 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. – Igby Largeman Apr 26 '10 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. – Nick Hodges Apr 27 '10 at 01:00
  • 4
    Somebody really should also point out the index over-run on the loop counter. ;) – Deltics Apr 27 '10 at 02:19
  • Oops! That's a mistake in my simplification, not an error in the original. Good catch. – Mason Wheeler Apr 27 '10 at 03:03
  • I agree with Nick. I reather have one waring to many, than one missing. And Mason in your case it is true that you will never get there. But there are cases when you may miss initialization. And warning is right on the spot in such cases. Turning warings off will leed to great pain later for sure. – Runner Apr 27 '10 at 07:27
  • And what about in the future if you suddenly decide to add a bit of code that messes with local even though flag might be true. You have to read that code /carefully/ to realise that initialisation isn't needed - maybe you won't be so careful when you need to get the code out of the door after a bug fix. – shunty Apr 29 '10 at 10:46
  • Since it's hard to find i shall add it here. The way to suppress the warnings (e.g. in [source code you didn't write](http://stackoverflow.com/questions/25746629/vcl-printers-pas888-w1025-unsupported-language-feature-custom-attribute)): **`{$WARN USE_BEFORE_DEF OFF}`** – Ian Boyd Apr 06 '15 at 15:14

5 Answers5

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.

Neil
  • 7,227
  • 5
  • 42
  • 43
  • 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: ;) – Deltics Apr 27 '10 at 00:14
  • Yeah, that's a decent performance optimization, but I don't see how it would eliminate the compiler warning. – Mason Wheeler Apr 27 '10 at 01:44
  • 2
    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. – Deltics Apr 27 '10 at 02:02
  • @Mason, it should eliminate the warning, because in the flag=true branch, you never try to access the value of `local`. Try it. – Neil Apr 27 '10 at 16:10
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!)

Chris Thornton
  • 15,620
  • 5
  • 37
  • 62
  • 2
    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. – Uli Gerhardt Apr 27 '10 at 08:36
  • 1
    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. – J... Nov 24 '14 at 13:49
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. :)

Deltics
  • 22,162
  • 2
  • 42
  • 70
  • 2
    Added remark : the two branches look different enough. Maybe split it in two functions : "if flag then DoSomethingSimple else DoSomethingExpensive;" – LeGEC Apr 27 '10 at 08:36
  • 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. :) – Deltics Apr 27 '10 at 21:05
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 :)

sabri.arslan
  • 548
  • 2
  • 14
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

Carl Manaster
  • 39,912
  • 17
  • 102
  • 155
Christopher Chase
  • 2,840
  • 6
  • 36
  • 57