Hi,

I want to bring up the warning of unreachable code. If one is implementing code according to some spec, e.g. RFC, or what else, it occurs that that requires a sanity check on user input. If it now happens that the type of variable by accident guarantees that this is true; this is fine, the compiler might optimize the check away. The compiler should not give a warning though IMHO, because the check is necessary according to spec, and it might become necessary if someone decides to change the type of the variable being checked to have a wider range. Furthermore, the warning is not suppressible.

Micha

Op Sun, 14 Oct 2007, schreef Micha Nelissen:

You mean this:

var b:byte;
    f:file;
begin
  {...}
  blockread(f,b,sizeof(b))l
  if (b<0) or (b>255) then
    begin
      writeln('Error: Invalid value');
      halt(255);
    end;
  {...}
end;

IMO the compiler in the case above totally correct to let you know the error condition can never occur. Even if a specification says 255 is the maximum, the code is totally bogus.

It is justified to warn, since in some cases an error situation can occur but the way the check is written causes a dead code check. We found a few of these in the compiler sources.

Daniël

Daniël Mantione wrote

It is justified to warn, since in some cases an error situation can occur but the way the check is written causes a dead code check. We found a few of these in the compiler sources.

A warning should always be fixable and doing so, turning it into correct code. The compiler is simply wrong here; my code is correct, so it should not complain.

It's not the compiler's responsibility that the code is correct; that is my responsibility, but if it knows for sure it can help me. It's a bit like the uninitialized variable warning, which the compiler can't know for sure as well.

Micha

Op Sun, 14 Oct 2007, schreef Micha Nelissen:

The warning is fixable: Remove the superfluous check.

It's not the compiler's responsibility that the code is correct; that is my responsibility, but if it knows for sure it can help me. It's a bit like the uninitialized variable warning, which the compiler can't know for sure as well.

If the compiler is sure your code is incorrect, it should give an error. However, unreachable code is just an indication something can be wrong and therefore a warning is more in place. There is still the issue of the false positives versus correct positives ratio, but this looks actually very good for this specific warning.

Daniël

Sigh. That's my point. If you were using smallint before, and someone changes the type to integer, then a bug is introduced; so you don't want to remove the check but the compiler can of course optimize it away.

Micha

Op Sun, 14 Oct 2007, schreef Micha Nelissen:

A solution is to make a dedicated type, and write down comments near its declaration.

If you remove the warning, code like:

if a>high(a) then

... no longer warns, which results in dead code in many situations.

I can follow your reasoning why you want the dead code, but on the other hand the warning is there with good reason.

Daniël

Daniël Mantione wrote

I can follow your reasoning why you want the dead code, but on the other hand the warning is there with good reason.

Maybe it would better be a hint.

Jonas

With the same reasoning you could make every warning a hint. Instead of if one usually uses asserts to check certain conditions.

I don't think so. While there may be false "uninitialised" warnings (because the compiler simply missed the initialisation somehow), every use of an uninitialised variable is an error.

However, not every case of an if-expression which can evaluated at compile time is an error. I even don't think the majority is.

Instead of if one usually uses asserts to check certain conditions.

These constructions can also be used instead of ifdefs, even though ifdefs are probably also more often used. Using if's instead is however not wrong imho.

Jonas

Florian Klaempfl wrote

> Maybe it would better be a hint. With the same reasoning you could make every warning a hint. Instead of if one usually uses asserts to check certain conditions.

Most are about bad code, that can be made better, AFAIK. E.g. 'an inherited method is hidden' needs a 'override;' or 'reintroduce;'.

Micha

Jonas Maebe wrote

Maybe it would better be a hint.

Seems reasonable to me.

The unused parameter hint is likewise also unavoidable with function callback implementations sometimes.

Micha

Joao Morais wrote

The compiler warns that the result wasn't assigned, but that function's result simply cannot be reached.

Why not make it a procedure ?

Micha

Daniël Mantione wrote

The warning is fixable: Remove the superfluous check.

All,

a new contribution to this theme. What about this warning:

{$mode objfpc}
uses sysutils;
function testresult: boolean;
begin
   raise exception.create('foo');
end;
begin
   testresult;
end.

The compiler warns that the result wasn't assigned, but that function's result simply cannot be reached.

Fixed in trunk when compiled with -Oodfa which will be probably a default in 2.4.0.

Because it is an abstract method.

Thanks. Now a note:

{$mode objfpc}
var
   vobj: tclass;
   vmethod: function: string of object;
begin
   vobj := tobject;
   vmethod := @vobj.classname;
   writeln(vmethod);
end.

Remove the comment and the note is also removed.

Joao Morais wrote

Remove the comment and the note is also removed.

sed 's/^Remove.*//'

There is a good reason for that the unreachable code is a warning: The compiler _changes_ your code by removing the if-branch. When it is a hint the compiler only has recognized a pattern, but it does not modify anything.

Peter

Peter Vreman wrote

There is a good reason for that the unreachable code is a warning: The compiler _changes_ your code by removing the if-branch. When it is a hint the compiler only has recognized a pattern, but it does not modify anything.

It "changes" it, but semantically it's the same right?

In your reasoning, the compiler should also emit a warning when it does (or would do) common subexpression extraction.

Micha

There were enough other reasons.

(or would do) common subexpression extraction.

Anyways, gcc warns similiar, even two times ;):

~> cat test.c
int f(unsigned char c)
{
if (c<0 || c>255)

    return 1;
  return 0;

}
~> gcc -c test.c
test.c: In function ‘f’:
test.c:3: warning: comparison is always false due to limited range of data type
test.c:3: warning: comparison is always false due to limited range of data type

So I guess the warning stays. We can discuss some extensions which makes it easier to code such restrictions like merging parts of the tue branch.

Yes, very annoying in gcc as well. It also warns when a variable is "shadowed", a similar annoying warning. GCC doesn't even have hints AFAIK.

Micha

So I guess the warning stays. We can discuss some extensions which makes it easier to code such restrictions like merging parts of the tue branch.

Could the warning not simply be switched off and on (or set to some kind of level) by a {$... line ?

-Michael

Op Mon, 15 Oct 2007, schreef Michael Schnell:

It is on the to-do list already. However, I don't think it'll be the end of the discussions; there will always be tension between the compiler being helpfull to signal dubious code, and the compiler generating too much noise.

Daniël

The compiler will be used in different ways by different people for different needs. Thus, viewpoints will differ ands that means that discussions about the pros and contras of a specific warning can be endless. The way out, I believe, is an infrastructure where individual warnings can be put on and off, like gpc and gcc have, e.g. see <http://gcc.gnu.org/onlinedocs/gcc-4.2.2/gcc/Warning-Options.html>. I believe I heard that it is on the todo list ?

Regards,

Adriaan van Os

Correct. It is already on the todo list for almost 10 years. For the current developpers it has very low priority, so don't expect anything soon.

Peter

Op Thu, 18 Oct 2007, schreef Adriaan van Os:

Indeed it is.

Daniël

The OP wanted to avoid a warning at a defined place in his code, because he is sure that the code he wrote __in these lines__ is correct. So being able to switch warnings on and off in the code would be easy to do and very helpful.

-Michael