Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Ondrej Pokorny]

On 12.05.2019 16:36, Jonas Maebe wrote:
Thanks. I have added these warnings to the compiler in r42047, and 
also the static/dynamic errors for Standard resp. Extended ISO Pascal.


By default, the warnings are enabled for
* enums
* boolean
* integer subrange types that don't span the entire range of their 
storage (e.g. 0..254, -128..0, ..., but not 
shortint/byte/smallint/word/...)


I do like this new warning.

I just don't understand why (for the else-block) it is shown at the end 
of the first statement, just before the semicolon (see the | where the 
warning is shown):


program Project1;
type
  TMyEnum = (one, two);
  procedure A;
  begin
  end;
var
  E: TMyEnum;
  I: Integer;
begin
  case E of
    one: ;
    two: ;
  else
    I := 15|;
    A;
  end;
end.

IMO it would make much more sense if it was shown in front of or behind 
the else keyword:


|else
else|

Ondrej

___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Martok]

Hi,

don't get me wrong, I like having something of mine included, but I'm genuinely
surprised you used it in the way I designed it.

As you said, the warning and code that is actually generated must agree,
otherwise they're useless. And there is a huge gap in the definition of the
Freepascal interpretation of what "all values of the type" even means, with the
code-as-written sometimes using one interpretation and sometimes the other.

On the topic of enums with assigned ordinals:

> I simply forgot to take them into account. I'm tempted to disable the 
> warning altogether for them, because
Please don't. Having sanity checks for "easy" types (where you don't really need
them) and not having them for "complicated" types (where they would likely catch
programmer errors) is not super helpful, and I would consider this as *worse*
than the status quo of not having warnings at all, because it would give a false
sense of safety ("My code compiles with -Sew? I must have done everything 
right!").
I'll never understand why Borland took that short cut for enumeration typeinfo
and got away with it.

> 1) Semantically, it would make sense to give a warning only if not all 
> explicitly defined enum values are covered
Agreed.

> 2) However, you still can't give an "unreachable code" warning if 
> there's an "else" in that case, because implicit values can also be 
> stored in the enum
Which takes us aall the way back to the original problem.

In my (and Borlands, and Microsofts) opinion, this program should compile
without raising the two new warnings (all named values are used AND else for
unnamed values 3 and 4), rangecheck as correct (3 in [2..5]), and be fully
defined at runtime. DFA should show that the else branch is always taken, which
may then mark the case labels as dead code.

program Project1;
{$mode objfpc}
type
  TMyEnum = (one = 2, two = 5);
var
  A: TMyEnum;
begin
  A := TMyEnum(3);
  case A of
one: Exit;
two: Exit;
  else
Writeln('Other');
  end;
end.

With R := Default(TMyRange), which assigns 0 [side note: why is the default
value an invalid value? That makes no sense at all], I would expect the same
results except for a range check warning at compile time (Range check error
while evaluating constants (0 must be between 2 and 5)).

I would *wish for* (and Borland documented this) the code to still be
runtime-safe even in $R- in this case, but I'm well aware that you and Florian
have already declined that. For completeness: subrange types silently upgrade to
their storage types if required, which to me implies also in comparisons, which
implies the "none of the above" label must always mean the entire storage type.


I'm still in favour of making an effort to formally define at least the core
elements of the Freepascal languages (syntax, statements, types, overloads,
resolution order, typeconv order, ...) with as few implementation quirks as
possible and then align the compiler to it (not the other way around). Having
less corner cases to juggle in one's mind may make things easier, and a formal
spec is also a good reference for future extensions to see if ideas are
orthogonal or not. But that's a whole different topic.


-- 
Regards,
Martok

___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Jonas Maebe]

On 12/05/2019 19:39, Ondrej Pokorny wrote:

On 12.05.2019 18:56, Jonas Maebe wrote:
As far as range checking and undefined behaviour is concerned, they 
do. I.e., you won't get undefined behaviour by assigning any value 
between low(enum)..high(enum) to them.


Very good, thank you. IMO this should be clearly documented to avoid 
confusion.


True.


And if they are valid they should not cause any runtime errors:

program Project1;
{$mode objfpc}
type
   TMyEnum = (two=2, five=5);
var
   E: TMyEnum;
begin
   E := TMyEnum(3);
   Writeln(E); // RunError (107)
end.

(From the above example one would say they are not valid.)


write(enum) is an FPC extension. It writes out the declared enum 
identifier. There is none in this case, so a run time error is appropriate.


There was a misunderstanding. I thought you meant with "implicit" values 
the "implicit default values" (=0):


program Project1;
{$mode objfpc}
type
   TMyEnum = (two=2, five=5);
   TMyObject = class
     E: TMyEnum;
   end;
var
   O: TMyObject;
begin
   O := TMyObject.Create;
   Writeln(Ord(O.E)); // R has an implicit default valuethat is invalid
   ReadLn;
end.

But you meant the implicit valid values (the holes). Now it's clear.


Ah, indeed.


Jonas
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Ondrej Pokorny]

On 12.05.2019 18:56, Jonas Maebe wrote:
As far as range checking and undefined behaviour is concerned, they 
do. I.e., you won't get undefined behaviour by assigning any value 
between low(enum)..high(enum) to them.


Very good, thank you. IMO this should be clearly documented to avoid 
confusion.


And if they are valid they should not cause any runtime errors:

program Project1;
{$mode objfpc}
type
  TMyEnum = (two=2, five=5);
var
  E: TMyEnum;
begin
  E := TMyEnum(3);
  Writeln(E); // RunError (107)
end.

(From the above example one would say they are not valid.)


Furthermore I strongly disagree with your (2) statement. On the 
contrary, if there are all explicitly defined cases covered and there 
is an else statement (like in the TestRanges program above), a 
warning about the unnecessary case statement is very needed because 
it is an "implementation detail" of the compiler if the else block 
will ever be executed or not (AFAIR you supported this).


The TestRanges program is not about an implementation detail, but 
about undefined behaviour. Those are two completely different things.


There was a misunderstanding. I thought you meant with "implicit" values 
the "implicit default values" (=0):


program Project1;
{$mode objfpc}
type
  TMyEnum = (two=2, five=5);
  TMyObject = class
    E: TMyEnum;
  end;
var
  O: TMyObject;
begin
  O := TMyObject.Create;
  Writeln(Ord(O.E)); // R has an implicit default valuethat is invalid
  ReadLn;
end.

But you meant the implicit valid values (the holes). Now it's clear.

Best
Ondrej

___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Jonas Maebe]

On 12/05/2019 18:30, Ondrej Pokorny wrote:

On 12.05.2019 17:45, Jonas Maebe wrote:
It's because Pred and Succ do not make sense with enumerations with 
holes in our opinion, and could be confusing. E.g. in your example, 
one could expect at first sight that succ(one) = two.


It depends if the holes are valid enumeration values or not.

Whether or not they are valid does not make it any less confusing.

If they 
are, Pred and Succ would make perfect sense - Delphi documentation 
explicitly states that Pred/Succ can and should be used to navigate to 
the holes:


I know, but I think that is not obvious from the declaration.

Yet, it still does not answer my question if the holes belong to the 
valid enumeration values from the FPC point-of-view or not.


As far as range checking and undefined behaviour is concerned, they do. 
I.e., you won't get undefined behaviour by assigning any value between 
low(enum)..high(enum) to them.


I simply forgot to take them into account. I'm tempted to disable the 
warning altogether for them, because
1) Semantically, it would make sense to give a warning only if not all 
explicitly defined enum values are covered
2) However, you still can't give an "unreachable code" warning if 
there's an "else" in that case, because implicit values can also be 
stored in the enum


That would make the warnings inconsistent.


Again, it all depends if the holes are valid enumeration values or not.

Your (1) suggests that they are not, whereas your (2) suggests they are.


My (1) simply means that if you declare an enum with three explicit 
values, those are probably the only ones you'll want to explicitly 
handle in a case-statement. But yes, you'll probably still want an 
else-statement to catch errors in case another value got in there 
somehow, so I'll leave the warning as it is now.


Btw. subrange types have the same problem with implicit values - the 
same with (2):


Subrange types don't have "implicit values". You have values that are 
valid/in the range, and you have values that are invalid/out of range. 
There is nothing in between, unlike with enumeration types with holes.



program TestRanges;
{$mode objfpc}
type
   TMyRange = 2..5;
var
   R: TMyRange;
begin
   R := Default(TMyRange); // assign implicit value


The bug here is that this compiles even when range checking is enabled. 
This stores an invalid value in R, hence anything that uses R after this 
statement has undefined behaviour. The compiler should warn about that 
without range checking, and given an error with range checking (like 
fore other range checking errors). An alternative is for Default() to 
return a different value than 0 in this case, but I don't know if that 
would fix more than it breaks.


Furthermore I strongly disagree with your (2) statement. On the 
contrary, if there are all explicitly defined cases covered and there is 
an else statement (like in the TestRanges program above), a warning 
about the unnecessary case statement is very needed because it is an 
"implementation detail" of the compiler if the else block will ever be 
executed or not (AFAIR you supported this).


The TestRanges program is not about an implementation detail, but about 
undefined behaviour. Those are two completely different things.


Someone in the future can 
decide that if the compiler finds an unnecessary else block, it will 
just ignore it. Don't call the warning "unreachable code" if you don't 
like it - call it whatever you want but keep it there.


The compiler will always warn before removing unreachable code and will 
call it as such.



Jonas
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Ondrej Pokorny]

On 12.05.2019 17:45, Jonas Maebe wrote:

On 12/05/2019 17:14, Ondrej Pokorny wrote:

On 12.05.2019 16:36, Jonas Maebe wrote:
Thanks. I have added these warnings to the compiler in r42047, and 
also the static/dynamic errors for Standard resp. Extended ISO Pascal.


Very nice.

One question about "C-style enumeration types" 
https://www.freepascal.org/docs-html/ref/refse12.html#QQ2-26-31


Do the holes between undefined enum values belong to the enumeration 
or not? Because:


It's because Pred and Succ do not make sense with enumerations with 
holes in our opinion, and could be confusing. E.g. in your example, 
one could expect at first sight that succ(one) = two.


It depends if the holes are valid enumeration values or not. If they 
are, Pred and Succ would make perfect sense - Delphi documentation 
explicitly states that Pred/Succ can and should be used to navigate to 
the holes:


"type Size = (Small = 5, Medium = 10, Large = Small + Medium);
Only three of these values have names, but the others are accessible 
through typecasts and through routines such as Pred, Succ, Inc, and 
Dec." - see 
http://docwiki.embarcadero.com/RADStudio/Rio/en/Simple_Types_(Delphi)#Enumerated_Types


But I am completely fine about your decision to disable Pred/Succ for 
C-style enumeration types (you did so only for non-delphi modes).


Yet, it still does not answer my question if the holes belong to the 
valid enumeration values from the FPC point-of-view or not.



I simply forgot to take them into account. I'm tempted to disable the 
warning altogether for them, because
1) Semantically, it would make sense to give a warning only if not all 
explicitly defined enum values are covered
2) However, you still can't give an "unreachable code" warning if 
there's an "else" in that case, because implicit values can also be 
stored in the enum


That would make the warnings inconsistent.


Again, it all depends if the holes are valid enumeration values or not.

Your (1) suggests that they are not, whereas your (2) suggests they are.

Btw. subrange types have the same problem with implicit values - the 
same with (2):


program TestRanges;
{$mode objfpc}
type
  TMyRange = 2..5;
var
  R: TMyRange;
begin
  R := Default(TMyRange); // assign implicit value
  case R of
    2..5: Exit;
  else // compiler warning: unreachable code
    Writeln('1: ', R);// unreachable code is still executed despite the 
warning!

  end;
  Writeln('2: ', R);
  ReadLn;
end.

Furthermore I strongly disagree with your (2) statement. On the 
contrary, if there are all explicitly defined cases covered and there is 
an else statement (like in the TestRanges program above), a warning 
about the unnecessary case statement is very needed because it is an 
"implementation detail" of the compiler if the else block will ever be 
executed or not (AFAIR you supported this). Someone in the future can 
decide that if the compiler finds an unnecessary else block, it will 
just ignore it. Don't call the warning "unreachable code" if you don't 
like it - call it whatever you want but keep it there.


Ondrej
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Jonas Maebe]

On 12/05/2019 17:14, Ondrej Pokorny wrote:

On 12.05.2019 16:36, Jonas Maebe wrote:
Thanks. I have added these warnings to the compiler in r42047, and 
also the static/dynamic errors for Standard resp. Extended ISO Pascal.


Very nice.

One question about "C-style enumeration types" 
https://www.freepascal.org/docs-html/ref/refse12.html#QQ2-26-31


Do the holes between undefined enum values belong to the enumeration or 
not? Because:


1.) The FPC documentation states they do not. See 
https://www.freepascal.org/docs-html/ref/refse12.html#QQ2-26-31
/"The Pred and Succ functions cannot be used on this kind of enumeration 
types. Trying to do this anyhow will result in a compiler error."/


It's because Pred and Succ do not make sense with enumerations with 
holes in our opinion, and could be confusing. E.g. in your example, one 
could expect at first sight that succ(one) = two.


2.) The Delphi documentation states they do. See 
http://docwiki.embarcadero.com/RADStudio/Rio/en/Simple_Types_(Delphi)#Enumerated_Types_with_Explicitly_Assigned_Ordinality
/"An enumerated type is, in effect, a subrange whose lowest and highest 
values correspond to the lowest and highest ordinalities of the 
constants in the declaration."/


3.) Your newly added warnings indicate they do.


I simply forgot to take them into account. I'm tempted to disable the 
warning altogether for them, because
1) Semantically, it would make sense to give a warning only if not all 
explicitly defined enum values are covered
2) However, you still can't give an "unreachable code" warning if 
there's an "else" in that case, because implicit values can also be 
stored in the enum


That would make the warnings inconsistent.


Jonas
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Ondrej Pokorny]

On 12.05.2019 16:36, Jonas Maebe wrote:
Thanks. I have added these warnings to the compiler in r42047, and 
also the static/dynamic errors for Standard resp. Extended ISO Pascal.


Very nice.

One question about "C-style enumeration types" 
https://www.freepascal.org/docs-html/ref/refse12.html#QQ2-26-31


Do the holes between undefined enum values belong to the enumeration or 
not? Because:


1.) The FPC documentation states they do not. See 
https://www.freepascal.org/docs-html/ref/refse12.html#QQ2-26-31
/"The Pred and Succ functions cannot be used on this kind of enumeration 
types. Trying to do this anyhow will result in a compiler error."/


2.) The Delphi documentation states they do. See 
http://docwiki.embarcadero.com/RADStudio/Rio/en/Simple_Types_(Delphi)#Enumerated_Types_with_Explicitly_Assigned_Ordinality
/"An enumerated type is, in effect, a subrange whose lowest and highest 
values correspond to the lowest and highest ordinalities of the 
constants in the declaration."/


3.) Your newly added warnings indicate they do. Because a warning is 
emitted if all well-defined values are used in the case statement:


program Project1;
{$mode objfpc}
type
  TMyEnum = (one = 2, two = 5);
var
  A: TMyEnum;
begin
  A := TMyEnum(3);
  case A of // compiler warning: Case statement does not handle all 
possible cases

    one: Exit;
    two: Exit;
  end;
  Writeln(A); // run-time error 107 - so maybe not a possible case 
after all?

  ReadLn;
end.

---

This dissimilarities should get resolved.

Ondrej

___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Jonas Maebe]

On 01/01/2019 22:10, Martok wrote:

The attached patch adds two messages inspired by C compiler's -Wswitch-enum and
-Wcovered-switch-default. Building on the recent label count refactoring, this
was fairly straightforward.

- If a case statement on an ordinal does not contain labels for all values of
the ordinal, and no else statement is given, raise a new warning (W6059). This
is actually defined as an error in ISO7185 and a dynamic-violation in IEC10206.

- If a case statement has labels for the entire range of the type, and an (now
never used) else statement is present, a warning "Unreachable code" is raised.


Thanks. I have added these warnings to the compiler in r42047, and also 
the static/dynamic errors for Standard resp. Extended ISO Pascal.


By default, the warnings are enabled for
* enums
* boolean
* integer subrange types that don't span the entire range of their 
storage (e.g. 0..254, -128..0, ..., but not shortint/byte/smallint/word/...)


The warning can be enabled for all ordinal types with the new -CC 
command line option. Like any other warning, it can be disabled with the 
corresponding -vm option (-vm6060 in this case).



Jonas
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Michael Van Canneyt]

On Wed, 2 Jan 2019, Martok wrote:


Am 02.01.2019 um 17:56 schrieb Michael Van Canneyt:

The compiler can prove that since the methods are private.
It can see no explicit calls are generated for it.


I thought so too, but even with added explicit calls, there is no warning.
Plus, it could only reason for this class if it was strict private, otherwise,
anyone in this module could access it.


When I said 'it can', I meant this is theoretically possible. 
I didn't mean to imply it actually does.


Michael.
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Jonas Maebe]

On 02/01/19 17:41, Martok wrote:

Wait - why does that code not raise a 5033 "Function Result does not seem to be
set"? Add a second property "Property MyOtherString : String Index 2 Read
GetString;", and now its provably wrong. No warning. This seems wrong,
considering what we just talked about on fpc-pascal.


It should print a warning if you compile with -Oodfa (or -O3, which 
includes -Oodfa). Without data flow analysis, the compiler cannot check 
whether the result is set on all possible paths, only whether it is set 
on at least one path.



Jonas
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Martok]

Am 02.01.2019 um 11:30 schrieb Florian Klämpfl:
> The compiler is for speed reasons not compiled with $R+/$O+, so the safety 
> else clause have its purpose, but they should
> throw an internal error instead.
And here we are at that other issue again. There is absolutely no guarantee the
safety clause is even present in the compiled ppc. The improved jumptable
generation actually makes it less likely! (the state of $R,$O doesn't change 
this)

So there's really 2 possible goals:
- safety against programmer error: a new stringtype is invented, and not
considered here. The coverage warning will tell the programmer, just as an IE
(maybe) would during testing -- it's a bug either way.
- safety against data error: def deserialized from damaged ppu? Undefined
behaviour, the else clause may not be present in the compiler binary, since all
legal values are handled. Which is exactly what the Unreachable Code warning
would tell you.


This segment of the compiler is specifically the kind of scenario that these
warnings are designed to catch, and also the reason why they are warnings, not
hints. Code that someone wrote and language spec do not agree, and the compiler
will do something that is *very likely* unintended.


-- 
Regards,
Martok


___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Michael Van Canneyt]

On Wed, 2 Jan 2019, Martok wrote:


Am 02.01.2019 um 11:19 schrieb Michael Van Canneyt:


Consider the following:

Type
   TMyClass = class
   Private
 function GetString(AIndex: Integer): string;
   Published
 Property MyString : String Index 1 Read GetString;
   end;

function TMyClass.GetString(AIndex: Integer): string;

begin
   case AIndex of
1 : Result:=GenerateSomestringvalue;
   end;
end;

I don't think there should be errors or warnings.


Good example.

Although... you *could* call GetString from some other class member function,
and at that point Result would be undefined. I don't think the compiler can
prove that?


The compiler can prove that since the methods are private.
It can see no explicit calls are generated for it.

Michael.
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Ralf Quint]

On 1/2/2019 2:18 AM, Sven Barth via fpc-devel wrote:
Am Mi., 2. Jan. 2019, 09:44 hat Martok > geschrieben:


* in ISO mode, there are 2 solutions: in Standard Pascal, not
covering the
entire type is a compile-time error. In Extended Pascal, this is a
runtime
error. Which one is followed usually?


ISO and Extended Pascal are handled as two separate modes by the 
compiler. So handling it according to the mode would probably be 
advisable.


+1

If someone wants to deliberately encumber themselves with using those 
standards, this might seem like a great idea. In reality, not so much.


Ralf



---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Benito van der Zander]

Wait - why does that code not raise a 5033 "Function Result does not seem to be
set"? Add a second property "Property MyOtherString : String Index 2 Read
GetString;", and now its provably wrong. No warning. This seems wrong,
considering what we just talked about on fpc-pascal.



Because there is no SetLength call in the function?


Am 02.01.19 um 17:41 schrieb Martok:

Am 02.01.2019 um 11:19 schrieb Michael Van Canneyt:


Consider the following:

Type
TMyClass = class
Private
  function GetString(AIndex: Integer): string;
Published
  Property MyString : String Index 1 Read GetString;
end;

function TMyClass.GetString(AIndex: Integer): string;

begin
case AIndex of
 1 : Result:=GenerateSomestringvalue;
end;
end;

I don't think there should be errors or warnings.

Good example.

Although... you *could* call GetString from some other class member function,
and at that point Result would be undefined. I don't think the compiler can
prove that?
Wait - why does that code not raise a 5033 "Function Result does not seem to be
set"? Add a second property "Property MyOtherString : String Index 2 Read
GetString;", and now its provably wrong. No warning. This seems wrong,
considering what we just talked about on fpc-pascal.


___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Martok]

Am 02.01.2019 um 11:19 schrieb Michael Van Canneyt:

> Consider the following:
> 
> Type
>TMyClass = class
>Private
>  function GetString(AIndex: Integer): string;
>Published
>  Property MyString : String Index 1 Read GetString;
>end;
> 
> function TMyClass.GetString(AIndex: Integer): string;
> 
> begin
>case AIndex of
> 1 : Result:=GenerateSomestringvalue;
>end;
> end;
> 
> I don't think there should be errors or warnings.

Good example.

Although... you *could* call GetString from some other class member function,
and at that point Result would be undefined. I don't think the compiler can
prove that?
Wait - why does that code not raise a 5033 "Function Result does not seem to be
set"? Add a second property "Property MyOtherString : String Index 2 Read
GetString;", and now its provably wrong. No warning. This seems wrong,
considering what we just talked about on fpc-pascal.

-- 
Regards,
Martok


___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Martok]

Am 02.01.2019 um 11:06 schrieb Bart:
> So now I will have to add a useless else statement for every case
> statement that uses e.g. integers,
Well, as Jonas argues on fpc-pascal, you are supposed to add useless statements
anyway, which the compiler will then 100% remove...

j/k.

In all seriousness though, that is why I didn't include all integers. But enums
and small ordinals are often used as a "what is this data" tag, where it is
usually an oversight if some value is not checked.
I'm already not super happy this also applies to Char (a OS_8 ordinal), because
it makes case-based regular language parsing quite noisy.


-- 
Regards,
Martok


___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Michael Van Canneyt]

On Wed, 2 Jan 2019, Mattias Gaertner via fpc-devel wrote:


> Warnings can always be disabled by {$warn NR off} or - I believe - a
> command line switch.
> One could also decide to have it be a default off warning for the
> non ISO modes (FPC, Delphi, MacPas) like some of the string
> conversion warnings. 


I would strongly advise to have it off by default for all but the ISO
modes.

You have perfectly valid code today (see the example of indexed
properties) which would start generating exceptions, and you should
not be forced to add a {$ } directive or an "else" clause to a case
statement for this.


I do hope you meant warnings, not exceptions.


Yes, sorry, typo :(





I agree this warning is very useful for enumerated types, but really
not for integers. There are too many cases where it would be a false
positive.


I have a lot of code with enums, where these warnings will force me to
do a lot of changes.


I can imagine it.

Maybe the warning should also depend on the range checking on/off switch.

Michael.
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Mattias Gaertner via fpc-devel]

On Wed, 2 Jan 2019 12:05:17 +0100 (CET)
Michael Van Canneyt  wrote:

> On Wed, 2 Jan 2019, Sven Barth via fpc-devel wrote:
> 
> > Am Mi., 2. Jan. 2019, 11:08 hat Bart 
> > geschrieben: 
> >> On Wed, Jan 2, 2019 at 9:44 AM Martok 
> >> wrote: 
> >>> - If a case statement on an ordinal does not contain labels for
> >>> all  
> >> values of  
> >>> the ordinal, and no else statement is given, raise a new warning  
> >> (W6059). This  
> >>> is actually defined as an error in ISO7185 and a
> >>> dynamic-violation in  
> >> IEC10206.
> >>
> >> So now I will have to add a useless else statement for every case
> >> statement that uses e.g. integers, or rewrite them to
> >> if..then..else or if value in [...]?
> >> And how will I mage this when my code runs on different
> >> architecture where the full range of the ordinal may differ?
> >>
> >> Please no (to this part of the proposal).
> >>  
> >
> > Warnings can always be disabled by {$warn NR off} or - I believe - a
> > command line switch.
> > One could also decide to have it be a default off warning for the
> > non ISO modes (FPC, Delphi, MacPas) like some of the string
> > conversion warnings.  
> 
> I would strongly advise to have it off by default for all but the ISO
> modes.
> 
> You have perfectly valid code today (see the example of indexed
> properties) which would start generating exceptions, and you should
> not be forced to add a {$ } directive or an "else" clause to a case
> statement for this.

I do hope you meant warnings, not exceptions.

 
> I agree this warning is very useful for enumerated types, but really
> not for integers. There are too many cases where it would be a false
> positive.

I have a lot of code with enums, where these warnings will force me to
do a lot of changes.

Mattias
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Michael Van Canneyt]

On Wed, 2 Jan 2019, Sven Barth via fpc-devel wrote:


Am Mi., 2. Jan. 2019, 11:08 hat Bart  geschrieben:


On Wed, Jan 2, 2019 at 9:44 AM Martok  wrote:


- If a case statement on an ordinal does not contain labels for all

values of

the ordinal, and no else statement is given, raise a new warning

(W6059). This

is actually defined as an error in ISO7185 and a dynamic-violation in

IEC10206.

So now I will have to add a useless else statement for every case
statement that uses e.g. integers, or rewrite them to if..then..else
or if value in [...]?
And how will I mage this when my code runs on different architecture
where the full range of the ordinal may differ?

Please no (to this part of the proposal).



Warnings can always be disabled by {$warn NR off} or - I believe - a
command line switch.
One could also decide to have it be a default off warning for the non ISO
modes (FPC, Delphi, MacPas) like some of the string conversion warnings.


I would strongly advise to have it off by default for all but the ISO modes.

You have perfectly valid code today (see the example of indexed properties)
which would start generating exceptions, and you should not be forced to 
add a {$ } directive or an "else" clause to a case statement for this.


I agree this warning is very useful for enumerated types, but really not for
integers. There are too many cases where it would be a false positive.

Michael.
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Bart]

On Wed, Jan 2, 2019 at 11:22 AM Florian Klämpfl  wrote:


> Which ordinal changes in FPC its range depending on the architecture?

Integer is not the same range on 16-bit platform IIRC?

-- 
Bart
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Marco Borsari via fpc-devel]

Il 01/01/2019 22:10, Martok ha scritto:


- If a case statement on an ordinal does not contain labels for all values of
the ordinal, and no else statement is given, raise a new warning (W6059). This
is actually defined as an error in ISO7185 and a dynamic-violation in IEC10206.



* in ISO mode, there are 2 solutions: in Standard Pascal, not covering the
entire type is a compile-time error. In Extended Pascal, this is a runtime
error. Which one is followed usually?


If we agree to check it only at runtime, then the error could be raised
everytime it is impossible to find a correspondence in the case 
statement and there is no an else clause, disregarding the completeness 
of the ordinal list and so including all types.

This way the check can be included in the $R switch.
Marco
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Florian Klämpfl]

Am 01.01.2019 um 22:10 schrieb Martok:

> 
> * Building the compiler itself cycles with -Sew, so it bails on the first
> occurrence of one of these issues - turns out they're all over the place.
> However... I think the warnings are correct and expose imperfect code here.
> Take this example (one of the first encountered):
> 
> stringtype is an enum and all values are used. There cannot be any other 
> values
> (as we have established last year). Therefore, the else clause is just as 
> wrong
> as an "if false then", which would be caught already.
> 
> * Adding to the previous, since it is now possible to discover forgotten items
> or later additions by the other warning, removing these 'safety' else-clauses
> would improve code quality.

The compiler is for speed reasons not compiled with $R+/$O+, so the safety else 
clause have its purpose, but they should
throw an internal error instead. Further, the advantage of an IE instead of a 
normal RTE is, that the IEs are pretty
unique so the source location of an IE can be identified very quickly, as the 
release compiler is not compiled with -gl.
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[LacaK]

>> - If a case statement on an ordinal does not contain labels for all values of
>> the ordinal, and no else statement is given, raise a new warning (W6059). 
>> This
>> is actually defined as an error in ISO7185 and a dynamic-violation in 
>> IEC10206.
> So now I will have to add a useless else statement for every case
> statement that uses e.g. integers, or rewrite them to if..then..else
> or if value in [...]?
> And how will I mage this when my code runs on different architecture
> where the full range of the ordinal may differ?
>
> Please no (to this part of the proposal).

+1 here or at least decrease severity from warning to some level that is
not by default printed in Messages window ...

-Laco.


___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Florian Klämpfl]

Am 02.01.2019 um 11:06 schrieb Bart:
> On Wed, Jan 2, 2019 at 9:44 AM Martok  wrote:
> 
>> - If a case statement on an ordinal does not contain labels for all values of
>> the ordinal, and no else statement is given, raise a new warning (W6059). 
>> This
>> is actually defined as an error in ISO7185 and a dynamic-violation in 
>> IEC10206.
> 
> So now I will have to add a useless else statement for every case
> statement that uses e.g. integers, or rewrite them to if..then..else
> or if value in [...]?

Actually, in a perfect world you would use a suitable range type.

> And how will I mage this when my code runs on different architecture
> where the full range of the ordinal may differ?
> 

Which ordinal changes in FPC its range depending on the architecture?
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Sven Barth via fpc-devel]

Am Mi., 2. Jan. 2019, 11:08 hat Bart  geschrieben:

> On Wed, Jan 2, 2019 at 9:44 AM Martok  wrote:
>
> > - If a case statement on an ordinal does not contain labels for all
> values of
> > the ordinal, and no else statement is given, raise a new warning
> (W6059). This
> > is actually defined as an error in ISO7185 and a dynamic-violation in
> IEC10206.
>
> So now I will have to add a useless else statement for every case
> statement that uses e.g. integers, or rewrite them to if..then..else
> or if value in [...]?
> And how will I mage this when my code runs on different architecture
> where the full range of the ordinal may differ?
>
> Please no (to this part of the proposal).
>

Warnings can always be disabled by {$warn NR off} or - I believe - a
command line switch.
One could also decide to have it be a default off warning for the non ISO
modes (FPC, Delphi, MacPas) like some of the string conversion warnings.

Regards,
Sven

>
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Michael Van Canneyt]

On Tue, 1 Jan 2019, Martok wrote:


Hi all,

The attached patch adds two messages inspired by C compiler's -Wswitch-enum and
-Wcovered-switch-default. Building on the recent label count refactoring, this
was fairly straightforward.

- If a case statement on an ordinal does not contain labels for all values of
the ordinal, and no else statement is given, raise a new warning (W6059). This
is actually defined as an error in ISO7185 and a dynamic-violation in IEC10206.

- If a case statement has labels for the entire range of the type, and an (now
never used) else statement is present, a warning "Unreachable code" is raised.

Both cases are clearly something where the compiler and programmer don't agree
on something, hence drawing attention to these situations.

The checks are enabled only for enumerated types and small (1-byte) integers. In
C, they are only for enumerated types, I added small types because they are
often used as tag types, where this check is extra useful.

Now, the RFC part. I have a few open questions...

* does it make sense to do that to all integral types? In ISO mode, it's
formally required for all case statements. I left it out for now as sometimes
case..of is used as a shorthand for multiple in..[range], there would be more
detections that look like false positives.


It does not make sense. Consider the following:

Type
  TMyClass = class
  Private
function GetString(AIndex: Integer): string;
  Published
Property MyString : String Index 1 Read GetString;
  end;

function TMyClass.GetString(AIndex: Integer): string;

begin
  case AIndex of
   1 : Result:=GenerateSomestringvalue;
  end;
end;

I don't think there should be errors or warnings.

Michael.
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Sven Barth via fpc-devel]

Am Mi., 2. Jan. 2019, 09:44 hat Martok 
geschrieben:

> * in ISO mode, there are 2 solutions: in Standard Pascal, not covering the
> entire type is a compile-time error. In Extended Pascal, this is a runtime
> error. Which one is followed usually?
>

ISO and Extended Pascal are handled as two separate modes by the compiler.
So handling it according to the mode would probably be advisable.

Regards,
Sven

>
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

Re: [fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Bart]

On Wed, Jan 2, 2019 at 9:44 AM Martok  wrote:

> - If a case statement on an ordinal does not contain labels for all values of
> the ordinal, and no else statement is given, raise a new warning (W6059). This
> is actually defined as an error in ISO7185 and a dynamic-violation in 
> IEC10206.

So now I will have to add a useless else statement for every case
statement that uses e.g. integers, or rewrite them to if..then..else
or if value in [...]?
And how will I mage this when my code runs on different architecture
where the full range of the ordinal may differ?

Please no (to this part of the proposal).

Bart
___
fpc-devel maillist  -  fpc-devel@lists.freepascal.org
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel

[fpc-devel] [Patch/RFC] Warnings for (in/over)complete case statements

[Martok]

Hi all,

The attached patch adds two messages inspired by C compiler's -Wswitch-enum and
-Wcovered-switch-default. Building on the recent label count refactoring, this
was fairly straightforward.

- If a case statement on an ordinal does not contain labels for all values of
the ordinal, and no else statement is given, raise a new warning (W6059). This
is actually defined as an error in ISO7185 and a dynamic-violation in IEC10206.

- If a case statement has labels for the entire range of the type, and an (now
never used) else statement is present, a warning "Unreachable code" is raised.

Both cases are clearly something where the compiler and programmer don't agree
on something, hence drawing attention to these situations.

The checks are enabled only for enumerated types and small (1-byte) integers. In
C, they are only for enumerated types, I added small types because they are
often used as tag types, where this check is extra useful.

Now, the RFC part. I have a few open questions...

* does it make sense to do that to all integral types? In ISO mode, it's
formally required for all case statements. I left it out for now as sometimes
case..of is used as a shorthand for multiple in..[range], there would be more
detections that look like false positives.

* in ISO mode, there are 2 solutions: in Standard Pascal, not covering the
entire type is a compile-time error. In Extended Pascal, this is a runtime
error. Which one is followed usually?

* Building the compiler itself cycles with -Sew, so it bails on the first
occurrence of one of these issues - turns out they're all over the place.
However... I think the warnings are correct and expose imperfect code here.
Take this example (one of the first encountered):

stringtype is an enum and all values are used. There cannot be any other values
(as we have established last year). Therefore, the else clause is just as wrong
as an "if false then", which would be caught already.

* Adding to the previous, since it is now possible to discover forgotten items
or later additions by the other warning, removing these 'safety' else-clauses
would improve code quality.


What do you think?

-- 
Regards,
Martok

From e32addb6583c8b752c168fe221385566499625bb Mon Sep 17 00:00:00 2001
From: Martok 
Date: Tue, 1 Jan 2019 18:59:56 +0100
Subject: [PATCH] Report warnings for incomplete and over-complete case
 statements

+ regenerated messages
---
 compiler/msg/errore.msg |   5 +-
 compiler/msgidx.inc |   5 +-
 compiler/msgtxt.inc | 816 
 compiler/ncgset.pas |  28 +-
 4 files changed, 440 insertions(+), 414 deletions(-)

diff --git a/compiler/msg/errore.msg b/compiler/msg/errore.msg
index b6448c25ba..449a9a1a4c 100644
--- a/compiler/msg/errore.msg
+++ b/compiler/msg/errore.msg
@@ -2350,7 +2350,7 @@ sym_e_type_must_be_rec_or_object=05098_E_Record or object 
type expected
 #
 # Codegenerator
 #
-# 06049 is the last used one
+# 06059 is the last used one
 #
 % \section{Code generator messages}
 % This section lists all messages that can be displayed if the code
@@ -2505,6 +2505,9 @@ cg_n_no_inline=06058_N_Call to subroutine "$1" marked as 
inline is not inlined
 % The directive inline is only a hint to the compiler. Sometimes the compiler 
ignores this hint, a subroutine
 % marked as inline is not inlined. In this case, this hint is given. Compiling 
with \var{-vd} might result in more information why
 % the directive inline is ignored.
+cg_w_case_incomplete=06059_W_Case statement has unhandled operand values
+% The case statement does not contain labels for all possible values of the 
operand, but not else statement is used.
+%
 %
 % \end{description}
 # EndOfTeX
diff --git a/compiler/msgidx.inc b/compiler/msgidx.inc
index 7100a9eee8..47752cfeb4 100644
--- a/compiler/msgidx.inc
+++ b/compiler/msgidx.inc
@@ -696,6 +696,7 @@ const
   cg_e_function_not_support_by_selected_instruction_set=06056;
   cg_f_max_units_reached=06057;
   cg_n_no_inline=06058;
+  cg_w_case_incomplete=06059;
   asmr_d_start_reading=07000;
   asmr_d_finish_reading=07001;
   asmr_e_none_label_contain_at=07002;
@@ -1106,9 +1107,9 @@ const
   option_info=11024;
   option_help_pages=11025;
 
-  MsgTxtSize = 82706;
+  MsgTxtSize = 82758;
 
   MsgIdxMax : array[1..20] of longint=(
-28,106,349,126,99,59,142,34,221,67,
+28,106,349,126,99,60,142,34,221,67,
 62,20,30,1,1,1,1,1,1,1
   );
diff --git a/compiler/msgtxt.inc b/compiler/msgtxt.inc
index baf2592f67..7ee50d0a45 100644
--- a/compiler/msgtxt.inc
+++ b/compiler/msgtxt.inc
@@ -869,493 +869,500 @@ const msgtxt : array[0..000344,1..240] of char=(
   'ion set: $1'#000+
   '06057_F_Maximum number of units ($1) reached for the current target'#000+
   '06058_N_Call to subroutine "$1" marked as inline is not inlined'#000+
-  '07000_DL_Starting $1 styled assembler parsing'#000+
-  '07001_DL_Finished $1 styled ass','embler parsing'#000+
+