Compiler warning "return value might be undefined"

Question

I often use code along the lines of:

function GetNumber(Handle : THandle) : Integer;
begin
FLock.BeginRead;
try
  if FMap.TryGetValue(Handle, Object) then
    raise EArgumentException.Create('Invalid handle');
  Result := Object.Number;
finally
  FLock.EndRead;
end;
end;

Unfortunately the compiler gives me a warning for all these methods:

[DCC Warning] Unit.pas(1012): W1035 Return value of function 'GetNumber' might be undefined

I know this warning, but in this case I can't see any reason for it at all. Or is there a scenario that I am missing that would result in an undefined result value? I understand the warning in the case of try..except but for try..finally it does not make sense to me.

Questions:

Is there any reason for the warning?
How can I get rid of it (moving the Result := Object.Number line out of the lock is not an option, and I want to avoid writing an completely unnecessary Result := 0 line at the top of each function)

Thanks!

+1. Unfortunately I had to deal with this a few times. You'll find some `Result := X; // Avoid compiler warning` in my code, some of them conditionally-compiled because of compiler changes between versions. Embarcadero should fix this bug because it's annoying! — Cosmin Prund, Jul 04 '11 at 10:50

score 5 · Answer 1 · answered Jul 04 '11 at 10:45

5

Is there any reason for the warning?

I can't see one but it is there because of the raise

How can I get rid of it (moving the Result := Object.Name line out of the lock is not an option, and I want to avoid writing an completely unncessary Result := 0 line at the top of each function)

Move the raise statement to a procedure of it's own.

function GetNumber(Handle : THandle) : Integer;
    procedure InvHandle;
    begin
        raise EArgumentException.Create('Invalid handle');
    end;
begin
    FLock.BeginRead;
    try
        if FMap.TryGetValue(Handle, Object) then
            InvHandle;
        Result := Object.Number;
    finally
        FLock.EndRead;
    end;
end;

answered Jul 04 '11 at 10:45

Mikael Eriksson

136,425
22
210
281

2

It's not really a problem with the raise. The compiler has long recognised that a code path with a raise absolves the coder of an obligation to assign to a return value. Try compiling the OP's code but with the Try/Finally removed! – David Heffernan Jul 04 '11 at 10:52
@David – There is no compiler warning without the try..finally. But I don't think it is a good solution to the problem at hand to remove the try..finally block. Moving the raise statement also removes the warning so I guess the bug in the compiler has to do with a combination of using raise within a try..finally block. – Mikael Eriksson Jul 04 '11 at 10:56
5

@Mickael I'm not suggesting that the solution is to remove the `try/finally`!!! The point I'm making is that this is clearly a compiler bug. – David Heffernan Jul 04 '11 at 10:57
@David - How can you be sure that it's a problem with `try/finally`but not with `raise`? Removing either one of them resolves the warning?! – Lieven Keersmaekers Jul 04 '11 at 11:43
@lieven without the raise it's a fundamentally different question. Anyway, whatever the cause is, something is wrong. – David Heffernan Jul 04 '11 at 11:46
@David: WOAH!! Removing `try/finally` removes resource protection of the lock! First exception, and you'll never unlock. – Disillusioned Jul 04 '11 at 16:50

David Heffernan · Answer 2 · 2011-07-04T12:04:04.207

3

That is a compiler bug. If the try/finally is removed then the warning is not emitted. The compiler has long been able to recognise that a raise absolves the coder of the obligation to assign the return value. For whatever reason the try/finally appears to confuse its analysis.

On second thoughts perhaps it's not a compiler bug. What if the code in the finally block stopped the exception propagating? Clearly there's not an except handler, but I rather suspect that one of the exception support routines in the System or SysUtils unit may be able to stop the exception progressing further.

edited Jul 04 '11 at 12:04

answered Jul 04 '11 at 10:48

David Heffernan

601,492
42
1,072
1,490

"but I rather suspect that one of the exception support routines in the System or SysUtils unit may be able to stop the exception progressing further" What makes you think so? And why would they do this? – jpfollenius Jul 04 '11 at 12:56
@Smasher I'm not so sure now. I was thinking `AcquireExceptionObject` would stop the exception progressing, but I think not. – David Heffernan Jul 04 '11 at 13:09
3

If there was no `raise` statement but simply `try …; Result := …; finally … end;`, there would be no Return Value Undefined warning, even if an exception would still be possible before reaching the Result assignment statement. – Andriy M Jul 04 '11 at 13:15
Removing `try/finally` removes resource protection of the lock! First exception, and you'll never unlock. Also if finally is ever able to block/stop the exception popagating - there's a far more serious problem at hand! – Disillusioned Jul 04 '11 at 16:52
6

@Craig I fear you have completely misunderstood the thrust of my post and wasted your valuable downvote. Of course I'm not advocating that OP removes the try/finally. That would be insane. The point I'm making is the interaction between the try/finally and the compiler warning. The warning is not emitted when there is not try/finally even though it has no impact on whether or not the return value is set. – David Heffernan Jul 04 '11 at 17:27

score -2 · Answer 3 · answered Jul 04 '11 at 10:33

-2

An assignment

Result := ...;

in the finally block is missing.

answered Jul 04 '11 at 10:33

Jiri Kriz

9,192
3
29
36

The additional result is required for an `except` block - not a `finally` block. – Disillusioned Jul 04 '11 at 16:53

Compiler warning "return value might be undefined"

3 Answers3