[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