Hi everyone,

So there have been a lot of issues going back and forth regarding case blocks when dealing with enumerations and what happens if the enum falls outside the valid range of values for some reason, because the internal domain check is sometimes omitted for performance reasons, since the value /shouldn't/ be invalid.  Thus, if it is invalid and a jump table is used, an access violation is raised.

Ultimately, the range check will likely not be included to safeguard against this, but there are still situations where an enumeration will take on an out-of-range value, like if it reads it from an external data file that is corrupted or of an incompatible version for the program.  So I would like to suggest a new intrinsic called InRange() or IsDomainValid() or something to that effect that takes an ordinal-type parameter and returns True if it contains a valid value for the enumeration (always True if it's a Byte or Word etc) and False if it is outside the valid domain, hence fulfilling the range check if it's something you really need.

The reason why I ask for an intrinsic instead of something like "if (Value >= Low(TEnumType)) and (Value <= High(TEnumType))" is because such code may be optimised out due to it logically being impossible for Value to be less than Low(TEnumType), for example, so it's harder than you think to check if the variable is in range.  Additionally, by having a generic intrinsic, it can generate more optimised code (e.g. the equivalent of "if (Value - Low(TEnumType)) < (High(TEnumType) - Low(TEnumType))", taking advantage of unsigned overflow if Value is less than the lower bound) and also to check for invalid values that fall within the maximum range (e.g. if you have something like "type TEnumType = (etZero, etOne, etSpecial = $FF);" and your Value holds the equivalent of $FE).

What are your thoughts? Would this be a decent compromise to fix the case range-check issue that keeps cropping up occasionally? I know it's causing some people to choose not migrate away from Delphi, and that is never really a good thing.  It should be easy enough to document as well, putting in an explicit warning for case to say that if the enumeration is out of range, it may cause a crash, and to prevent it, use the intrinsic to check beforehand.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

I thought this was already discussed and accepted as desirable ? If memory serves well, the only discussion was about form...

Why would this stop anyone from migrating away from Delphi ?

Delphi has the exact same problem: an enumerated may be out of range. I had it on multiple occasions when database data was invalid.

It may not have the case jump table problem, but other than that it is not better than FPC.

Michael.

I don't recall a conversation where such an intrinsic is seen as desirable, but if such a construct is desirable, then that's a good thing since it will help patch up this problem.

As for the Delphi problem, it's second-hand information I heard from a friend who uses Delphi at work and something about not handling out-of-range enumerations was a deal-breaker from not using FPC.  I may have some details wrong though.  But all that side, would development of an intrinsic be an acceptable solution, and what name are we going for? (One has to be careful we don't break existing code if possible)

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

J. Gareth Moreton wrote

I don't recall a conversation where such an intrinsic is seen as desirable, but if such a construct is desirable, then that's a good thing since it will help patch up this problem.

The conversation is more than a year old and I already posted a patch - also more than a year ago. I haven't received any feedback from the compiler developers since I uploaded the patch. Maybe you will have more luck.

Last email in the ML:
https://lists.freepascal.org/pipermail/fpc-devel/2019-May/040862.html

Patch: https://bugs.freepascal.org/view.php?id=33603

Best
Ondrej

See this thread:

Michael.

For me this patch is perfectly OK.

Michael.

Aah, thank you.

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Don't forget to check the post of Ondrej.

Personally, I think using 'as' to convert an integer to an enum with range check is still the best approach. It does not require new intrinsics (far too many of those already) and does what it says. To simply check if an integer is a valid enum, I think 'is' would be equally suitable.

Michael.

The latest patch in https://bugs.freepascal.org/view.php?id=33603 supports bot AS and IS operators. Yes, the first patch was only about the AS-operator and the issue summary is about AS only as well. You know
- nobody is perfect and I didn't realize the IS-operator was suitable there too in the very beginning. The mind&development progress can be read from the ML thread.

Ondrej

I wish you did a little bit in the compiler as well and not only RTL :) I know, one person can't do everything. Thanks!

Ondrej

I like the idea of using existing operators since that avoids extra intrinsics and the risk of breaking code that uses a function with the same proposed name.

The "as" operator is possible for assignment, although I'm not the biggest fan of run-time errors (although they have their uses).  The "is" operator sounds like a good partner as well, like how as and is are used to check if an object belongs to a particular class family.

e.g. "If (Value is TEnumType) then" returning False if it is outside of the range.

Though I'm only a developer and not one of the moderators or administrators, the patch in principle has my blessing.  There might be options for optimisation further down the line, but we'll get to that later!

Gareth aka. Kit

I should probably test this myself, but if you, say, read a value from a file directly into a variable of type TEnumType, will your patch return False for "if (Value is TEnumType) then" if Value, despite being TEnumType, contains an invalid value?  No problem if it isn't, just that you would have to read an Integer-type first then typecast it.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Yes:

var
   Value: TEnumType;
begin
   Value := TEnumType(-1);
   IsValid := Value is TEnumType; // IsValid gains false

The compiler may not do any optimizations here (like return always true if the left side value is the enum type at the right side). This should be clearly stated and documented if the feature is added to FPC.

This is similar to the object-is operator that gets evaluated as well even if the type of the left-side value is the type at right side:

var
   Value: TPersistent;
begin
   Value := TPersistent(TObject.Create);
   IsValid := Value is TPersistent; // IsValid gains false

Ondrej

That is exactly how it should behave, because the left-side value may not be valid, and the is operator is meant to help confirm that.

In the meantime, I've updated your patch to work with the current trunk: https://bugs.freepascal.org/view.php?id=33603#c117040

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Whenever you need an exception like this, it means that the behaviour does not fit in the language. As a result, it would make the language (more) inconsistent and harder to reason about, even independently of optimizations. E.g. if Value is inside a bitpacked array or record, IsValid will may well be true instead of false, because a bunch of bits were thrown away during the assignment. This shows that the only place where the checking can reliably happen, is during the initial conversion (which should have been done with "as" in this scenario).

And yes, it also complicates compilers/optimizers, because you have to make sure you handle this special case everywhere, now and forever. E.g. it would make it impossible to add range annotations for enum types in LLVM bitcode without adding volatile load hacks all over the place.

This is an invalid program. If you compile with -CR, the program will abort with an error when the typecast is performed, because it will get replaced by an "as" operation. In that sense, "integer as enum" would indeed be somewhat similar, and -CR might even be extended to perform the same replacement of explicit typecasts with "as" operators for these types.

As an example of an operation on the resulting "Value" that is already undefined: if you would call a TPersistent virtual method on it, and whole-program optimization devirtualised that call, it may call the "correct" method of TPersistent instead of using the VMT of whatever other class instance type Value points to.

Invalid data means undefined behaviour, always. "is" is not a special case that is immune to this. And e.g. in the context of generics, simplifying/removing such checks where possible would probably be quite desirable.

As to your patch itself: why do you not directly compare the tconstexprint values directly, and use the svalue/uvalue fields instead?

Jonas

Ondrej Pokorny wrote

The conversation is more than a year old and I already posted a patch - also more than a year ago. I haven't received any feedback from the compiler developers since I uploaded the patch. Maybe you will have more luck.

FWIW: I've been flying this patch in the snapshot builds ever since, and nobody complained. So it's probably not immediately broken ;-)

The problem is much bigger than enumerated types though, all subrange types are fundamentally incompatible between FPC and Borland. I don't remember if your patch handles those correctly as well? That probably needs a few test cases?

The main problem, Jonas, is that there will be times where the enumeration will take on a value that is outside of its domain, and often through ways that we cannot help, such as external data being in the wrong format.  I'm all for compiler optimisations assuming that the value is in range for case blocks and conditional shortcutting, but there needs to be a reliable way to get a "Yes" or "No" answer as to if it's valid or not that won't fall foul of these assumptions, hence my suggestion of an intrinsic or, as with Ondrej's patch, the "is" operator.

I suppose with the bitpacked version... admittedly I can't think of a construct off-hand that would break it, but if someone knows of one, then I can definitely evaluate it.

All in all though, we need /something/, because one can argue about language semantics until the cows come home, but here is a reproducible situation that cannot be reliably accounted for without ditching enumerations entirely, which will only serve to obfuscate third-party code for no obvious reason.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

No, this is a perfectly valid program with a perfectly defined behavior! It is a perfectly valid program even with -CR. When I use -CR I expect an exception and I will handle it - in this case I don't need the is-check:

var
   Value: TPersistent;
begin
   try
     Value := TPersistent(TObject.Create); // or: TObject.Create as
TPersistent (without -CR)
   except
     Writeln('Wrong value supplied! Exit.');
     Exit;
   end;
   // do something
   Readln;
end.

A good real-word example is the cast from a pointer:

program Project1;
uses Classes;
var
   Value: TPersistent;
   P: Pointer;
begin
   P := TObject.Create;
   Value := TPersistent(P);
   if Value is TPersistent then
     Writeln('Value is TPersistent')
   else
     Writeln('Value is not TPersistent');
   Readln;
end.

Yes, I agree that calling an (unavailable) method on an object casted to a wrong class is an invalid operation. But this is not what I did in my example. Preventing calls of unavailable methods or access of unavailable fields is exactly what I the IS-operator is for.

Invalid data means undefined behaviour, always. "is" is not a special case that is immune to this.

We are again at the very fundamental question "what invalid data is". Storing a TObject in TPersistent is not an invalid operation IMO. Both are class objects and both can be checked for inheritance with the IS-operator. Calling methods, accessing fields etc. on the TObject-value in TPersistent-variable is invalid, indeed. But not a simple IS-operator.

What I understand as invalid for the IS-operator is e.g. trying to call it for an interface in TPersistent variable:

var
   Value: TPersistent;
   P: Pointer;
   I: IUnknown;
begin
   I := TInterfacedObject.Create;
   P := I;
   Value := TPersistent(P);
   if Value is TPersistent then

And e.g. in the context of generics, simplifying/removing such checks where possible would probably be quite desirable.

Not really. The IS-operator returns false for NIL that is a valid value, so you cannot remove such checks:

var
   Value: TPersistent;
   IsValid: Boolean;
begin
   Value := nil;
   IsValid := Value is TPersistent; // IsValid gains false - you cannot
simply replace this check with true
   Writeln(IsValid);
   Readln;
end.

Ondrej

This discussion is useless unless the compiler can assure that a check will happen for every possible (initial) conversion/assignment:

E.g. in

1.) Read(MyEnumValue);
2.) TStream.Read(MyEnumValue, SizeOf(MyEnumValue));

etc. etc.

Your philosophy effectively forbids the use of the above methods for enums/subranges, because once some value is inside MyEnumValue without a range check, you never ever can check if it is valid or not.

Even this does not help:

case Integer(MyEnumValue) of
   Integer(Low(TEnumType))..Integer(High(TEnumType)): Writeln('valid');
else
   Writeln('invalid');
end;

It cannot be used because casting MyEnumValue back to Integer ("Integer(MyEnumValue)") is undefined for invalid MyEnumValue values. Thanks - we came to nothing.

Big wow: you say "the only place where the checking can reliably happen, is during the initial conversion". FPC currently doesn't offer any checking for conversions to enum. So every read from external data must currently consist of these 3 steps:
1.) read the value as an integer
2.) check manually that the integer value is between low and high 3.) assign the integer value to the enum value
This results in a lot of bloated code. And with the "undefined" IS-operator it doesn't get any better. Only the 2nd point gets a little bit shorter.

And yes, it also complicates compilers/optimizers, because you have to make sure you handle this special case everywhere, now and forever. E.g. it would make it impossible to add range annotations for enum types in LLVM bitcode without adding volatile load hacks all over the place.

I have to admit that I do not understand what you mean with the text above. It's probably my lack of knowledge.

As to your patch itself: why do you not directly compare the tconstexprint values directly, and use the svalue/uvalue fields instead?

Because I missed that they can be directly compared directly.

Ondrej

I dare say, I would advise you make any new changes to my "AS-IS-enum-04.patch", since that adds an internal error to one of the case blocks (mostly to suppress a compiler warning) and ensures it merges cleanly with the trunk.

I am with you that we need /something/ to determine if an enumeration is in range or not because of the common problem of reading from a file.

Invalid data happens - to actually enforce that all behaviour is undefined is VERY bad practice.  There are some applications where the program must be able to handle and recover erroneous inputs, and a data file is one such source of errors - for example, a program writes an enumeration to a file, and in a later version of the program, you add a new value to the enumeration, and then try to read that file in the old version of the program.  You could argue that it was a poor design choice to not include a version field so you don't read such a value in the first place, but it's too late to change that, and it still doesn't trap the case where the file has been deliberately modified in an attempt to crash the program.  If a developer wishes to make their software robust, we have no right to forbid it.  Honestly, it just discourages the language's use.

The "as" and "is" operators are the only 'exceptions' to the language that would be required here.  Range checks need not be included in other conditions, and a single exception can be documented.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

To clarify my stance on all of this... when enumerated types are entirely internal within a program, then there is indeed no need for constant range checks like with case blocks because the value is almost always guaranteed to be valid unless you do something weird like Pointer typecasting, using 'absolute' or manipulating the value with assembly language, in which case, all bets are definitely off.

However, when you start interfacing with something external, such as a file or a third-party library, then there is a chance that you will get an invalid value through no fault of your own, but you still need to be able to handle it in some way, because crashing with an access violation is wholly unacceptable behaviour (e.g. a server application that serves many millions of clients, or the firmware on a flight computer).  Normal range checks won't work because the compiler believes the variable can never take on those invalid values and so will optimise them out.  In this case, there needs to be an explicit exception to the rule so we can take an alternative branch - all that is needed is a result that says "this enumeration contains an invalid value" and handle it as desired, either via a specific exception being raised (with "as") or a Boolean condition (with "is").  There doesn't need to be any other kind of range check; if the programmer wants to handle /specific/ invalid values, then they should add them as new entries in the enumeration.

I understand the desire and benefits of language purity, but when multiple programmers fall foul of a particular feature with no clear workaround, then it needs to be evaluated.  Saying "it's not a bug, it's a feature" and "invalid data equals undefined behaviour" will just frustrate people and potentially cause them to switch to an alternative development tool.  If you ask around, some people have already done this or have chosen not to adopt FPC because of it.  Yes, FPC is not perfect and never will be, but why shouldn't we strive for it anyway?

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

If you want to extend -CR to perform "integer as enum" on enum(integer), please do it consequently and perform "integer as ShortInt" also on ShortInt(integer) - do range checking on explicit typecasts for all simple types, not only enums. People will love you for this:

var
   S: ShortInt;
   I: Integer;
begin
   I := 130;
   S := ShortInt(I);
end;

shall now raise a range check error with -CR. You can even optimize the compiler to verify constants on compile-time:

var
   S: ShortInt;
begin
   S := ShortInt($FF);
end;

should not compile any more with -CR. Good idea.

BTW I was always convinced that an explicit typecast switches off range checking. And now I open the documentation
https://www.freepascal.org/docs-html/prog/progsu65.html and I read this: (citation)
/If, at run-time, an index or enumeration type is specified that is out of the declared range of the compiler, then a run-time error is generated, and the program exits with exit code 201. This can happen when doing a typecast (implicit or explicit) on an enumeration type or subrange type./

This means that this program is documented to exit with code 201:

program Project1;
uses Classes;
{$R+}
var
   B: Boolean;
begin
   B := Boolean(3);
   Writeln(B);
end.

Why doesn't it?

Ondrej

More fun:

program Project1;
{$R+}
type
   TSubRange = 1..2;
var
   S: TSubRange;
begin
   S := Default(TSubRange);
   Writeln(S);
end.

compiles and runs just fine. Why?

This does not compile:

program Project1;
{$R+}
type
   TSubRange = 1..2;
var
   S: TSubRange;
begin
   S := 0;
   Writeln(S);
end.

Ondrej

Jonas Maebe wrote

Invalid data means undefined behaviour, always. "is" is not a special case that is immune to this.

Don't you really see the need to handle invalid data with a /defined/ behavior? Enum and subrange types /can/ store invalid data and they /do/ store invalid data. Be it implicit assignments in object creation, clearing records with Default(TMyRecord), reading records from streams etc etc. The issue is here and it needs some solution. If you don't like "is" for this purpose, why not to introduce a compiler intrinsic Valid(MyValue) that would check if MyValue is within the allowed range of its type?

Or do you really think Valid(MyValue) should always return true and thus be replaced with the true constant at compile time?

Ondrej

Of course I would do it if I decided to work on it. But no, I won't work on the compiler bugs/extension/anything any more. It's just a waste of time.

One reason is that I don't see any reasonable philosophy behind it. As a result FPC is a mixture of Delphi compatibilities and incompatibilities, applied optimizations and missing optimizations, applied extensions and missing extensions that even contradict one another sometimes. I see FPC as a personal project of the compiler team who has an absolute right of taking decisions about it. So I just accept it as-is.

The second reason is that the compiler people are very busy (which I perfectly understand) and it's hard to get some feedback even when fixing bugs.

On the other hand it's not that bad all in all. FPC still is a good compiler and I prefer it over Delphi. And I have the source code of it, so if it really matters, I can patch it for myself.

Ondrej

On 2019-07-03 08:58, Ondrej Pokorny wrote:

Hi,

Well, I'm not sure to which extent the comment was meant seriously (no smiles around), but I'm not convinced that explicit typecasts should ever result in errors (even a warning would be very questionable, IMHO), because explicit typecasts belong to the category "strong tools in responsibility of the user" (similarly to working with pointers, embedded assembler, etc.).

.
.

Obviously, this doesn't happen with explicit typecasts and I believe that it's correct.

Tomas

In general, assuming P was a pointer initialized elsewhere (meaning you really didn't know what it was), why wouldn't you just write this:

  if TObject(P) is TPersistent then
    Writeln('Value is TPersistent')
  else
    Writeln('Value is not TPersistent');
  Readln;

Behaves as you'd expect, and doesn't require an additional variable.

Sometimes I can be too passionate for my own good, and I want FPC to continue being a great tool, without problems that prove to be a game-breaker.

I would like to continue working on this particular issue and hopefully find an answer that everyone is happy with.  There are ways that an enumeration can take on an invalid value, but let's at least give the programmer an option of detecting the validity and not be forced to let their program enter an undefined state, because sometimes that is simply not an acceptable path.

And I have been saying this to myself, but it might be a bit too radical... "It's time FPC left the shadow of Delphi and stepped into the light!" - let it become its own thing (but with Delphi compatibility when it's needed with {$MODE DELPHI}).

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Ben Grasset schrieb am Mi., 3. Juli 2019, 18:00:

The code by Ondrej was to prove a point not to have the shortest variant.

Regards,
Sven

So is the "(Value as TEnumType)" and "if (Value is TEnumType)" acceptable solutions to do a range check then the programmer absolutely needs it?  What do the administrators feel?  Not to be the bearer of bad news, but I feel like there's some disagreement.

Note that things like the range check on case blocks will not be modified and don't need to be.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

My point is that is impossible to do so, so trying to do it in a way that works in some/most cases, is much more dangerous than categorically refusing to try to do it, as it creates a false sense of security.

For example:

{$mode objfpc}
type
  tc = class
    a: array[1..2000] of longint;
  end;
  tcc = class of tc;
  trec = record
    p: pointer;
  end;
  prec = ^trec;
var
  c: tc;
  p: prec;
  cc: tcc;
begin
  new(p);
  cc:=tc;
  p^.p:=pointer(cc);
  c:=tc(p);
  writeln(c is tc);
end.

This will print true, even though it's obviously bogus.

Enum and subrange types /can/ store invalid data and they /do/ store invalid data.

The same goes for classes, and by extension for any pointer type.

Because it means that not a single transformation or check in the compiler can assume that MyValue contains a valid value. Either the compiler can assume validity and the manual check does not make any sense, or the compiler cannot assume it and then the language's type system becomes meaningless (*), and bitpacked arrays/records should be removed from the language.

The whole point of a type system is that
a) the programmer is responsible to ensure variables are filled with valid values when bypassing the type system (explicit typecasts, inline assembly, variant records, absolute variables, "external" aliases, ...) b) the compiler is responsible in case the type system is not bypassed (**): either it gives a compile-time error, or it generates run-time checks that detect issues (which can be disabled for performance reasons)

Or do you really think Valid(MyValue) should always return true and thus be replaced with the true constant at compile time?

Yes.

That said, these assumptions can be used in multiple ways. E.g. when clang appeared, many programmers hated it because it generated non-working code for many programs that worked with GCC. The reason was that clang aggressively exploits undefined behaviour for optimization purposes, and even explicitly models it internally. The original code may even have worked on one GCC version/platform, but have been broken on another combination.

Then clang introduced another project: ubsan (undefined behaviour sanitizer). Everywhere the compiler makes an assumption about something, this one will insert checks so that at run time it verifies that the assumptions the compiler made are not invalidated by undefined behaviour, and if not it aborts the program. A ubsan-like mode for FPC would definitely be useful, even if not all undefined behaviour in FPC is also undefined in Delphi (or maybe especially because of that reason). In a way, the -CR option is already a bit like that (and it is indeed very useful).

Jonas

(*) The situation with the enumerations and subrange types is more complicated than that. The reason is (as Martok pointed out in a previous thread) that Delphi does explicitly consider the entire range of representable bitpatterns valid for a subrange/enumeration type. This leads to some inconsistencies and weird range checking behaviour, but with that definition an "IsValid()" intrinsic for enumeration and subrange types would make sense within the type system.

Normally, we would simply follow the Borland behaviour in TP/Delphi modes and use the more strict definition in FPC modes. Unfortunately, that is not possible for this difference. See
https://forum.lazarus.freepascal.org/index.php/topic,45507.msg322059.html#msg322059 for why.

(**) The Default() intrinsic is indeed a nasty in this context. I'm personally inclined to consider it equivalent to declaring an uninitialsed global variable of the same type, or to having a field of that type in a class. I.e., the value has been cleared, but is still "uninitialised" (similar to how FPC warns about an uninitialised use when reading a global variable without first writing to it).

The reason is not because that is more convenient to support the existing FPC behaviour, because I actually started implementing a "SafeDefault" intrinsic after the previous thread on this topic, and it's not possible (just like with having defined behaviour for invalid data). In this case, the issue is with variant records. You can even have two overlapping fields of enumeration/subrange types that have mutually exclusive ranges.

You have such a validity check for objects in FPC on the compiler side: there are compiler intrinsics fpc_check_object & fpc_check_object_ext that can be enabled with -Cr and -CR. So actually you do try to check the validity of objects.

But I see that this is an internal feature of FPC that is not to be executed manually and there is no assurance of correctness - it is only a helper feature for debugging.

With this said, I do accept that it is a no-go to check a valid enumeration value against its own type in the code (MyEnum is TMyEnum). We are left with a check of a simple value (ordinal, enumeration, subrange) against a different enumeration/subrange type:

(MyInt is TMyEnum)
(MyEnum2 is TMyEnum)
etc. as a shorthand for ((Ord(Low(TMyEnum)) <= MyInt) and (MyInt <= Ord(High(TMyEnum))).

This is still better than nothing.

+++++++

BUT (big BUT): I suggest to do an enumeration/subrange validity check on the compiler side when range checking is used. This is similar to the current fpc_check_object intrinsic.

When range checking is used, this check should be injected (at least) before every case-of and the new is/as operators if they are replaced with true. When range checking is disabled, the check should not be for range checking:

1.) case MyEnumValue of // <<< here an enumeration validity check should be injected
2.) IsValid := MyEnumValue is TEnumValue; //  <<< replace with TRUE but still insert a validity check intrinsic if range checking is used. If not replaced with true, do not insert validity check (so that false can be returned without a runtime error).

IMO such a solution is acceptable for both sides:
Your point: The clarity of the language will stay untouched because the new validity check will be only informative and used only for debugging without any assurance of correctness. (You see the difference: MyEnumValue is TEnumValue will raise an exception for an invalid value and not return false! False will be returned only for (MyInt is TEnumValue).)
My point: We will get an information about invalid enum values (range check error) instead of "undefined behavior" when debugging - and we will be able to find code where invalid enum values can occur and fix the code where these values were assigned. Although this information may not be (theoretically) absolutely accurate (the same as fpc_check_object is not absolutely accurate), it will still be pretty accurate and it will be a big help during the debugging.

Ondrej

Mucking around with pointers will always produce unpredictable code if you don't take care in what you're doing (and in your example, it uses tools that already exist), but Jonas, I have to protest your logic.  Saying it's safer to refuse to do it at all instead of doing something that works part of the time is not always a sound design philosophy.  Enums taking on invalid values is something that can and does happen, but by adamantly refusing to allow the programmer to check its validity and demanding unpredictable program behaviour is not something I or others can accept.  Or saying that all bets are off if a file is corrupt or has been tampered with... some programs, by design, are simply not allowed to crash.  If a sensor on an aircraft returned an invalid value (say for the sake of argument it was an undercarriage feedback that returned "Locked", "Stowed" and "Off" (stowed but without hydraulic pressure)) and the flight computer crashed because the developers were not allowed to create defined behaviour for invalid data, would that be acceptable for you?

There have been so many bug reports from all sorts of developers who have complained abut this behaviour and telling them that "it's not a bug" and not even offering them a workaround from undefined behaviour is both arrogant and ignorant.

When entirely self-contained within a program, the type system is fine, and there's no need to do range checks on enumerations, but when you have an external interface, such as a data file or a third-party library (either written in FPC or another language), there needs to be a way to check if the enum is within range or not, and using the "is" operator in this way is a perfectly reasonable way of doing things.  Because it's just an integer value at the lowest level, it will return a sane answer unless you go out of your way to try to break it.

As for the thing about keeping Pascal pure and consistent... it never was.  You have code blocks wrapped in "begin...end" and then you have the oddball that's "repeat...until", which is a carry-over from some dialects of BASIC.  And then you have things like the "class procedure Etc; static;" directive when a slightly better design might have been "static procedure Etc;", but it's too late to change that now.  Sometimes you have to make design choices based on realistic needs rather than an ideal.

It can be enough to put in the documentation for 'case' something like the following, since it is these that cause crashes: "*WARNING:* Case blocks do not check to see if an enumerated value falls within the valid range, and passing an invalid value can trigger an access violation.  To avoid this behaviour when the validity of the input is uncertain, perform a separate check with "(Value is TEnum)" - a result of False indicates an invalid value." - that is all that would be needed.  If you're not happy with using "is" for this purpose, then I would like to offer my original option of an intrinsic that takes on the form of a Boolean function: "if IsValidEnum(Valid) then ...".  Note I do not consider trapping EAccessViolation to be an acceptable solution because it is very inefficient with performance and is too broad an exception that usually indicates a serious problem (e.g. insufficient system memory).

Ultimately, being forced to ditch enumerations for classic C-style constants defined against the Integer type (the only real workaround) only makes the code more difficult to understand, and a more junior developer may think to replace them with enumerations under the philosophy of improving readability, not realising that it will cause the compiler to shortcut the safety checks.  If I were a senior project developer, I simply could not take such a risk and would discard FPC completely for a tool what will not cause this problem (or has a documented workaround).

You could consider this an ultimatum, but I know it will be pointless to do that because you'll just ignore me.  So I'm pleading, Jonas... there needs to be a way to check if an enumeration contains an invalid value to avoid diving into the realm of undefined behaviour.  Just because you wouldn't use it doesn't mean we won't.

Gareth

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Correct.

But the whole idea of the intrinsic for enumerated is simply to save typing. basically

if (myvalue>=ord(Low(TMyType))) and (MyValue<=Ord(High(TMyType))) then

becomes

if IsValid(myvalue,TMyType) then // or myvalue is TMyType or IsValid(MyValue)

The languague is full of such constructs already: "is" and "as". "for...in" SizeOf(), default(), assert().
These are all convenience methods, none of them are infallible: "Is" and "As" can also fail. Default can also fail. 'implicitly initialized global variables' can also fail.

This has not stopped anyone from adding them.

So I see no reason why we should not add this convenience method: It is not at odds with the fact that the compiler can assume that an enumerated value is in range.

Michael.

It's not so much convenience... it's more the fact that the compiler can and will optimise out "if (MyValue>=ord(Low(TMyType))) and (MyValue<=Ord(High(TMyType))) then" because it has every reason to assume that MyValue cannot possibly take on a value that's less than Low(TMyType) or greater than High(TMyType), hence it gets shortcut and assumed to always be True.

When MyValue takes on an invalid value due to being read from a file, for example, then that's where problems occur because there's no clear-cut way to catch it.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

No. MyValue is an integer. Hence the ord().

Obviously the check must be done before the typecast, not after...

Let me make that more explicit:

if MyIntValue is TMyEnum then
   MyValue:=TMyEnum(MyIntValue);
or
MyValye:=MyIntValue as TMyEnum;

That's why I said it it is not at odds with the fact that the compiler can assume MyValue is always valid.

Michael.

Ah, my apologies, Michael.

I can see the issue of it being a convenience thing, but given that many programmers have fallen foul of the lack of range checking in case blocks, the typecasting necessary to avoid the problem is cumbersome, adds a performance penalty and, ultimately, implies you should just not use enumerations at all in that instance.

When an issue is raised from many different sources, it usually means something is counterintuitive.  We only know about it because we've done a lot of compiler development.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

With this sentence you forbid storing or communicating enumerated values in any way: file, database, over network. It can be used only in a computer program and never leave the context of the running program under any form. Because as soon as it is somehow communicated, there is a chance it becomes invalid in return communication.

Additionally you must then also abolish typecasting to an enumerated (or pointers to enumerated values), since that also can be a source of invalid values for an enumerated.

Or are you realy advocating we write code such as

Case MyInteger of
   0 : MyEnum:=ValueOrd0
   1 : MyEnum:=ValueOrd2;
else
   someError
end;
etc, whenever an integer must be changed to an enumerated ?

IMHO you would reduce the usabilty of enumerateds to almost zero by doing so.

So you may want to reconsider, since in my experience most enumerateds at one point do end up outside the program and typecasts are equally widespread.

These patterns are so ubiquitous it is worthy of an intrinsic.

Michael.

Please don't put words in my mouth Michael or twist me up with word play.  I have never said I want to forbid the storing or communication of enumerations, and if people start using my argument for the exact opposite of what I'm trying to advocate and smugly say "I rest my case", then you can have my resignation. Please clarify with me before making such assumptions next time.

Because enumerations may take on an invalid value in these media (there's no real way you can eliminate this possibility), you need a reliable means to check their validity.  Many of FPC's optimisations assume that a variable of type TMyEnum is valid, so it skips range-checking for performance reasons among others, and instructions like "if Value < Low(TMyEnum) then" are always assumed to be False.  Sometimes even the typecasting gets shortcut so you have to be very careful.  I suggested one should not use enumerations as the only way to get around that, which is not ideal at all, but to use enumerations on top of Jonas' stance of essentially demanding undefined program behaviour for invalid values is just plain dangerous for real-world applications.

My original suggestion was to use an intrinsic like "if IsValidEnum(Value) then" as an efficient and robust way to check if the variable contains a valid value for the relevant enumeration, once which won't be shortcut by the compiler's assumptions as with case blocks and the if-statement in the last paragraph.  It was then brought to my attention that a variant had already been proposed with the "is" operator, which in my eyes is a little bit better because it doesn't introduce a new intrinsic that may clash with existing function names in some projects.

But to avoid a fight or anything of the sort, given that external data is read as an enumeration and may contain invalid data (which to give a concrete definition, is an integer value that doesn't map to any element in the enumeration), and it won't be caught by case blocks and can trigger an access violation if a jump table is employed (this is how the compiler currently behaves), what would you propose be the best solution?

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Okay, I've done some preliminary testing, and "if Value < Low(TMyEnum) then" and "if Value > High(TMyEnum) then" work as expected and aren't optimised out, even when using -O4 -Cr.  So admittedly there is still a way to check to see if the enumeration is within the valid range, at least for now.  I'm not sure if the compiler will be changed so this would be optimised out (in which case, an intrinsic or "is" would be necessary).

I do think one needs to put in a clear warning in the documentation for 'case' so developers who do fall foul of the slightly counterintuitive behaviour have a means to work around it.

This was my test, written in a way so I couldn't be told off for using 'absolute', pointers or assembly language and is comparable to a real-world example of reading an enumeration from a data stream:

----

program EnumTest;
uses
   Classes;
type
   TMyEnum = (meOff = 1, meLowered, meStowed);
const
   ByteSequence: array[0..5] of Byte = (0, 2, 4, 127, 128, 255);
var
   Stream: TMemoryStream;
   EnumVal: TMyEnum;
   X: Integer;
begin
   Stream := TMemoryStream.Create;
   try
     { Fill the memory stream }
     Stream.SetSize(Length(ByteSequence));
     Stream.Write(ByteSequence, Length(ByteSequence));
     Stream.Seek(0, soBeginning);
     { Okay, now read back the values }
     while Stream.Read(EnumVal, 1) > 0 do
     begin
       if EnumVal < Low(TMyEnum) then
         WriteLn('Below valid range')
       else if EnumVal > High(TMyEnum) then
         WriteLn('Above valid range')
       else
         WriteLn(EnumVal);
     end;
   finally
     Stream.Free;
   end;
end.

----

If I make the enumeration non-contiguous and try to call WriteLn(EnumVal) with an invalid value that is otherwise still within the lower and upper bounds, then I do get an exception, which is to be expected - "EInOutError: Unknown Run-Time error : 107" - the only real fault with it is that it's listed as an unknown error.  If an intrinsic or "is" is accepted as a convenient shortcut for the bounds check, it should probably catch values that fall within these gaps.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Note, with "is" or an intrinsic that only does a range check, it can be made very fast when platform-specific.  For example, for "Result := (Value is TMyEnum);", under x86, if "l" is the Low value and "h" is the High value, then the assembly becomes this (remember that l and h are constants, so h - l is also constant and determined at compile time):

MOV   REG, Value
SUB   REG, l
CMP   REG, (h - l)
SETNA Result

If l = 0, which is very common for most enumerations, then the assembly is even simpler, not requiring a temporary register:

CMP   Value, h
SETNA Result

For "If (Value is TMyEnum) then", the SETNA instruction becomes a conditional jump.

In this instance, the intrinsic's function is similar to how "Inc(X);" is a shorthand for "X := X + 1;", there for convenience more than anything but traditionally able to produce more optimal code.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Jonas Maebe schrieb am Do., 4. Juli 2019, 21:21:

Then how would you read data from e.g. a stream into an enum or subrange if the stream may contain invalid data?

Regards,
Sven

My aplogies. I didn't mean to put words in your mouth, I really understood your argument as a case for not using enumerations in most cases.

It seems I misunderstood you.

My thinking exactly. With the exception that the construct must be used on the integer value instead of the enumerated, as the enumerated is expected to have valid values only, and the check must be done before converting.

Well, I still favour:

Type
   TMyEnum = (one, two, three);
Var
   I : Integer;
   M : TMyEnum;
begin
   I:=0;
   if I is TMyEnum then
     M:=TMyEnum(I); // Will be executed
   I:=3;
   if I is TMyEnum then
     M:=TMyEnum(I); // Will NOT be executed
   // and
   I:=0;
   M:=I as TMyEnum; // Will work
   I:=3;
   M:=I as TMyEnum; // Will raise out of range run-time error
end.

IMO, if the compiler insists on assuming that an enumerated value contains only valid values (an assumption I endorse, as it allows better optimization) then it should also give the programmer the means of easily assuring this, because the language also allows for many cases where the assumption may be invalid: typecasts, variant records.

I think the above proposal does exactly that.

In this, I would definitely exclude enumerateds that have explicitly assigned values: str does not handle them, getenumename etc. also do not work: They are in effect simply integer constants. (if I had my way I would even remove them from the language).

So using the above construct on such an enum can lead to a compiler error, because the compiler cannot check anyway: better a clear error than undefined behaviour.

Michael.

On the contrary, the compiler can check enums with holes with no problem at all. They have valid low/end boundaries. And the holes in between are valid values as well, even if they don't have names assigned. (E.g. the warnings in case-of are enabled for enums with holes as well!)

The 02.patch and 05.patch in
https://bugs.freepascal.org/view.php?id=33603 handle enums with holes correctly. (Only the 03.patch disabled IS/AS for holes with enums in OBJFPC mode as a complementary feature, but I removed it consequently.)

See Jonas' answer about enums with holes:
https://lists.freepascal.org/pipermail/fpc-devel/2019-May/040868.html

Citation:

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.

+++

So there is no actual need or benefit to disable this feature for enums with holes.

+++

Btw. why nobody commented on my latest suggestion in https://lists.freepascal.org/pipermail/fpc-devel/2019-July/041499.html ? IMO this is a valid solution for both sides.

Ondrej

Eh ? Not in my book, if the compiler allows that it is simply wrong IMO.

An enumerated enumerates the allowed values: no holes allowed.

What you describe, I would name a range, not an enumeration.

So there is no actual need or benefit to disable this feature for enums with holes.

See above.

The way I read jonas' argument:

TMyEnum = (one, two=3,three);
A:=TMyEnum(1) ;

will pass unchallenged.

I don't see the point of allowing this. for me, "A" has an invalid value.

Note:
This is my opinion, it may be at odds with what the compiler actually implements :)

What part exactly ? The additional range check for a case in case of range checking ? I don't see why you need it, since in case range checking is enabled, the assignment of the variable should already have ensured that there are only valid values in it. But I have no strong opinion on this.

As for (MyEnumType1Value is TMyEnumType2) : this is also a good suggestion.

Michael.

Read:
http://docwiki.embarcadero.com/RADStudio/Rio/en/Simple_Types_(Delphi)#Enumerated_Types_with_Explicitly_Assigned_Ordinality

Yes.

I don't see why you need it, since in case range checking is enabled, the assignment of the variable should already have ensured that there are only valid values in it.

The compiler can never catch all kinds of assignments and do range checking on them (e.g. FillChar, Read, pointer-writes, etc. etc.). So a range check when accessing a value is needed.

It's the same reasons FPC has fpc_check_object and falls into the same philosophy.

If I take your statement "in case range checking is enabled, the assignment of the variable should already have ensured that there are only valid values in it." to the extreme, it implies that the creation of an object with a subrange value always ends up with a range check error:

program Project1;
type
   TMyClass = class
   private
     FSubRange: 2..4;
   end;
   TMyRecord = record
     SubRange: 2..4;
   end;
   PMyRecord = ^TMyRecord;
var
   C: TMyClass;
   R: PMyRecord;
begin
   C := TMyClass.Create; // << range check error here
   R := AllocMem(SizeOf(TMyRecord)); // << range check error here
end.

Ondrej

As I said, my opinion may be at odds with what the compiler actually implements.

Note that the explanation is IMO fuzzy, not to say contradictory:

'An enumerated type defines an ordered set of values by simply listing identifiers that denote these values'

is at odds with (under the heading 'Enumerated Types with Explicitly Assigned Ordinality'):

'An enumerated type is, in effect, a subrange'

But opinions may vary.

If memory serves well, FPC introduced this (before delphi) to be able to import C enums with assigned values.

Point taken, so your proposal makes sense...

Michael.

Michael Van Canneyt schrieb am Fr., 5. Juli 2019, 10:47:

If memory serves well, FPC introduced this (before delphi) to be able to import C enums with assigned values.

Correct. That is also why we allow both "=" and ":=" for the value assignments. Because Delphi only supports one of the two and we picked the other back then. ;)

Regards,
Sven

No contradiction here: the corresponding implied subrange goes from lowest to highest assigned value, which is 0..n for dense enumerations and low(consts)..high(consts) for Enumerated Types with Explicitly Assigned Ordinality. There is nothing special about the holes, they just have no names. This is also how the Delphi debugger displays them.

Again, I recommend reading the TP5 and TP7 books, which contained the *rationale* for most of the type system. That got left out when they rewrote the content for Delphi Help.

This is the conclusion from Jonas' arguments, which he explicitly confirmed when we had the discussion two+ years ago.

You're also missing another important type of 'communication': calling non-FPC APIs or even just FPC libraries of possibly different version (see: Run-Time Packages!)

Anything that even looks like a subrange cannot be used for these interfaces.

IMO The whole point of an enumerated is to have the names for all allowed values. i.e. I would write 'by simply listing all allowed identifiers' in the above.

But I will not argue the point, it is largely a matter of personal interpretation of the intended use of an enumerated.

But I will definitely adapt the FPC documentation to be more clear about the actual behaviour of the compiler.

Michael.

I am only a hobbyist, so take this for what it's worth. I am very puzzled by Gareth's insistence on this issue. It seems to me that, if there is a chance that a value read from some external source might have a value outside the range of an enum, then enum is not the right data type for that operation. Use an appropriate integer type instead.

Tony

Ignorant Delphi core developers have never checked how FPC does things before implementing their own solution to keep compatibility ;) And then they added ARC, then they removed ARC, they then added 0-based strings, then they killed the product (AppMethod) they made it for... Unfortunately the 0-based string features made it into FPC in TStringHelper as well...

Ondrej

APIs falls under 'communicating'. Database is an API. Library is an API, REST is an API: you communicate the value to a system not under your control.

Anything that even looks like a subrange cannot be used for these interfaces.

Yes, if you adhere to a strict view.

But IMO such a strict stance is untenable: you cannot ensure correct values in all cases even within the confines of the language (as demonstrated before).

So, in my book, an intrinsic to check for a correct value in the places where an invalid value can enter the system is a welcome language feature.

Put another way:
If the programmer is responsible (correctly so) for ensuring correct values, then I think the language must give him the tools to fulfill his responsability in a convenient manner. Hence the need for an intrinsic.

Michael.

TStringHelper is for delphi compatibility, so there is no choice.

All in all, I am still of the opinion that the whole SysUtils unit is a monster that should be taken apart and its functionality spread over various units: StrUtils, DateUtils, IOUtils, Exceptions, Helpers.

Doing so would also release us from the need to be Delphi compatible in those units. Alas, this is meanwhile a huge undertaking.

Michael.

I know, I am not blaming you (or anybody else) for doing so.

Still, in my opinion the 0-based string feature is the best candidate for knowingly breaking Delphi compatibility. The same as the core team communicated that the inline var declaration from Delphi 10.3 is not welcome in FPC.

Yes, that is actually a good compromise: to move the Delphi-compatibility TStringHelper into a different unit (e.g. DelphiHelpers.pas) and offer a similar helper with 1-based strings in Helpers.pas. Yes, there is too much to do :(

Ondrej

You may use enumerations for these interfaces - you may just not fill them directly but through an integer variable in between when reading from outside.

IMO this is not a problem - most modern interfaces work like this anyway. E.g. for the database you have the TField.AsInteger:

if MyField.AsInteger is TMyEnum then
   MyEnumValue := TMyEnum(MyField.AsInteger)
else
   // do something in case of invalid value

The above code will work perfectly and safely.

For REST or SOAP APIs etc. you have a JSON or XML reader that again returns an Extended (JSON) or string (XML) so again this is no problem at all.

Only the old-fashioned direct reads (from files, streams etc.) must be done with an extra integer variable in between. But that is safer anyway because e.g. if you add new enumeration values the SizeOf(TMyEnum) may increase and then you have a big problem anyway.

So all-in-all the 2 features solve the problem:
1.) IS/AS operators. They will save some typing (check for low-high bounds). 2.) Additional compiler built-in enumeration value checks when range-checking is on. They will help to find places where an invalid enumeration value may occur and so the program code can be fixed during debugging.

Ondrej

Hi Michael,

I'm glad we agree on the "is" operator.  Personally I would like to extend it so it accepts "if (I is TMyEnum) then" when I is also of type TMyEnum, since it may be the case that you're reading I directly from a stream, either individually or as part of a record, in which case converting I from an Integer to TMyEnum is a little inconvenient.  When you get down to the compiled assembly language it shouldn't really matter anyway.

For enumerations with holes, having "is" return False if a value is within one of these holes would require more complex code, at least to work out how to programmatically cover these holes with conditions and not pollute the simpler version where all elements are contiguous.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Martok schrieb am Fr., 5. Juli 2019, 11:10:

Huh? The comment regarding dynamic packages is nonsense. The compiler and RTL check that the checksums of the involved interface sections match and thus there is no problem there.

Regards,
Sven

J. Gareth Moreton schrieb am Fr., 5. Juli 2019,
14:02:

It might not matter from the assembly point of view, but it matters from the language point of view. When we say that enum values only contain valid values then "myenum is TMyEnum" *must* always evaluate to true, because otherwise there's an exception of the rule that enums only contain valid values.

Regards,
Sven

Sven Barth wrote

Huh? The comment regarding dynamic packages is nonsense. The compiler and RTL check that the checksums of the involved interface sections match and thus there is no problem there.

Does that cascade? If so, that's better than Delphi, nice :)

Michael Van Canneyt wrote

But IMO such a strict stance is untenable: you cannot ensure correct values in all cases even within the confines of the language (as demonstrated before).

Correct. Even if certain (exactly two) people here believe differently.

So, in my book, an intrinsic to check for a correct value in the places where an invalid value can enter the system is a welcome language feature.

In my book it's absolutely insane that this is needed *just because* FPC wants to be different, but yes, it is required.

Then you're not using them *in* the interface ;-)

But I will not argue the point, it is largely a matter of personal interpretation of the intended use of an enumerated.

Well, yes. Since any argument from authority is discarded, what remains is personal interpretation.

But I will definitely adapt the FPC documentation to be more clear about the actual behaviour of the compiler.

And this is why we can't have nice things.

Please don't get me wrong (really!), this is nothing against your work specifically, but that the documentation has to follow the implementation and the implementation follows no particular philosophy at all is what got this interpretation of the language into this mess in the first place.

FPC is in dire need of a language specification effort. But I don't think anyone who would be willing to drive it cares enough at that point.

In this situation, the exception is kind of needed, and was suggested for this purpose in the first place.  If you read an enum from a stream or any kind of external source, then there is a chance it will take on an invalid value and needs to be trapped. I don't think anyone wants to be forced to ditch enumerations for integers and C-style constants so as to not fall foul of this quirk.  "is" is to catch an enumeration taking on an invalid value, thus allowing customised error handling to be executed, so other expressions can be optimised on the assumption that the enum is definitely valid by that point.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Agreed. Ondrej

For records you could use such a workaround:

program Project1;
{$mode objfpc}
{$modeswitch advancedrecords}
type
   TMyEnum = (one, two, three);
   TMyRecord = record
   public
     function IsValid: Boolean;
   public
     case Boolean of
       False: ( // real values
         Enum: TMyEnum;
       );
       True: ( // used for interface
         EnumIntf: Integer;
         {$IF SizeOf(Integer)SizeOf(TMyEnum)}
           {$ERROR Size mismatch}
         {$ENDIF}
       );
   end;
{ TMyRecord }
function TMyRecord.IsValid: Boolean;
begin
   Result := EnumIntf is TMyEnum;
end;
var
   R: TMyRecord;
begin
   // fill R with data ...
   Randomize;
   FillChar(R, SizeOf(R), Random(High(Byte)));
   // check validity
   if R.IsValid then
     Writeln('R is valid')
   else
     Writeln('R is not valid');
   Writeln(R.EnumIntf);
   ReadLn;
end.

Best
Ondrej

Just one point from current compiler implementation:

in trunk/fpcsrc/compiler/ninl.pas (around line 3180)

              in_pred_x,
              in_succ_x:
                begin
                   set_varstate(left,vs_read,[vsf_must_be_valid]);
                   resultdef:=left.resultdef;
                   if is_ordinal(resultdef) or is_typeparam(resultdef) then
                     begin
                       if (resultdef.typ=enumdef) and
                          (tenumdef(resultdef).has_jumps) and
                          not(m_delphi in current_settings.modeswitches) and
                          not(nf_internal in flags) then
                         CGMessage(type_e_succ_and_pred_enums_with_assign_not_possible);
                     end

This means that using pred() or succ() intrinsics on enumerated types with holes will generate a Compile Time Error.

To be consistent, I would propose that we also generate a Compile Time Error if 'is' or 'as' keyword is used on such a type, unless there is a code that really check that the value is not in one of the holes ...

Pierre

Only for non-Delphi modes, see: not(m_delphi in
current_settings.modeswitches)

To be consistent, I would propose that we also generate a Compile Time Error if 'is' or 'as' keyword is used on such a type, unless there is a code that really check that the value is not in one of the holes ...

See this thread:
https://lists.freepascal.org/pipermail/fpc-devel/2019-May/040865.html

Ondrej

Martok schrieb am Fr., 5. Juli 2019, 14:54:

As loading dynamic packages is an expensive operation anyway I do indeed intend to check the checksum of all involved packages and units for consistency. I've been bitten often enough in Delphi by this. I don't want the same problems in FPC...

Regards,
Sven

That seems fair.  The main problem is what happens if, when allowed, Prec or Succ are used on such a type on an element that is adjacent to one of these holes.  Should it skip to the first element at the other end of the hole or raise an exception? Granted, what happens if you use Prec or Succ on the first or last element respectively?

I can theoretically design an algorithm to cover these gaps with Boolean conditions for the sake of "is", but it's a little bit complex.

Sorry to bring you in on this particular point, Pierre, but if you call "Value is TEnum" and Value is of type TEnum but contains an invalid value (due to being read from an external stream, for example), should it return True or False?

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Le 05/07/2019 à 17:42, J. Gareth Moreton a écrit :

My answer would be:

in FPC modes, simply generate a CompileTimeError as above,

in Delphi mode, as Delphi states stat the holes are also valid values of the range, then indeed, pred() and succ() can do the simple thing they do when there are no holes, and 'is' or 'as' should then also do the same, i.e. only check that the value is inside the range defined by the lowest and highest values!

But on course this means that in Delphi mode,
even after validating with 'is' you could still have a valid enum value that has no name, i.e. that is in one of those black holes ...

I don't need to tell you that I do have a preference for the ordinary Free Pascal way!

Pierre

Pierre Muller schrieb am Fr., 5. Juli 2019, 17:51:

I think Gareth meant his question for enums without holes ;)

Regards,
Sven

Hey there - sorry if I'm not explaining myself too well.

My question would be this... if Value is a variable of type TEnum that is invalid (either because it is out of range or contains a 'hole' value, but more specifically the former), should "Value is TEnum" return True, False or an exception?  If it's an exception, then "Value is TEnum" will only ever return True or raise an exception if Value is of type TEnum.  If it returns False instead, then it's a nice and efficient check to see if Value is valid or not, which is the main point of contention around this topic... how can we reliably detect an invalid value and not let the program slip into an undefined state?

For Prec and Succ to return an exception if the next value is a hole, that will certainly be easier to program, since then the code can be equivalent to this:

----

function Succ(EnumVal: TEnum): TEnum; inline;
begin
   Result := TEnum(Ord(EnumVal) + 1);
   if (m_delphi in current_settings.modeswitches) and
     not (Result is TEnum) then
       raise RunTimeError201;
end;

----

Of course the code would be generated directly and not have the check for Delphi mode (it'll just remove the condition and exception raise completely) but you get the idea.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

I have to strongly disagree here. We need the IS/AS operators to prevent undefined behavior. And if undefined behavior is only outside the low/high bounds of the enumeration values and not within the holes - then the IS/AS that checks only the bounds is very consequent indeed. It's also consequent with the case-of statement for an enum with holes. (See the thread I linked before.)

Ondrej

I now did a proof of concept for that task myself:

=== code begin ===

program tptrhlp;
{$mode objfpc}
{$modeswitch typehelpers}
uses
   Classes, SysUtils;
type
   TStreamHelper = type helper for TStream
   public
     generic function ReadEnum<T>(out aEnum: T): Boolean;
   end;
generic function TStreamHelper.ReadEnum<T>(out aEnum: T): Boolean;
var
   buf: array[0..SizeOf(T) - 1] of Byte absolute aEnum;
   tmp: LongInt;
begin
   if Read(buf[0], SizeOf(buf))  SizeOf(buf) then
     Exit(False)
   else begin
     case SizeOf(T) of
       1:
         tmp := PByte(@buf[0])^;
       2:
         tmp := PWord(@buf[0])^;
       3:
         tmp := LongWord(PWord(@buf[0])^) or (LongInt(PByte(@buf[2])^)
shl 16);
       4:
         tmp := PLongWord(@buf[0])^;
     end;
     Result := (tmp >= Ord(Low(T))) and (tmp <= Ord(High(T)));
   end;
end;
type
   {$PACKENUM 1}
   TMyEnum = (
     meOne,
     meTwo,
     meThree
   );
var
   s: TMemoryStream;
   e: TMyEnum;
   b: Byte;
begin
   s := TMemoryStream.Create;
   try
     try
       Writeln(SizeOf(TMyEnum));
       b := 1;
       s.Write(b, SizeOf(b));
       b := 3;
       s.Write(b, SizeOf(b));
       s.Position := 0;
       if not s.specialize ReadEnum<TMyEnum>(e) then
         raise EStreamError.Create('Failed to read enum value');
       Writeln('Read value: ', e);
       if not s.specialize ReadEnum<TMyEnum>(e) then
         raise EstreamError.CReate('Failed to read enum value');
     except
       on e: Exception do
         Writeln(e.ClassName, ': ', e.Message);
     end;
   finally
     s.Free;
   end;
end.

=== code end ===

Note 1: Needs today's trunk cause I fixed generic methods in helpers in mode ObjFPC
Note 2: Similar code can also be done for ranges (though there the sizes 5 to 8 need to be handled as well)
Note 3: I didn't test whether this works correctly on Big Endian systems Note 4: The compiler will optimize away the unneeded case branches :)

Regards,
Sven

[snip]

[snip]

=== code end ===

Oh and once I've integrated Ryan's implicit specialization support the code can become this (or at least I hope so ^^'):

=== code begin ===
       if not s.ReadEnum(e) then
         raise EStreamError.Create('Failed to read enum value');
       Writeln('Read value: ', e);
       if not s.ReadEnum(e) then
         raise EstreamError.CReate('Failed to read enum value');

=== code end ===

Regards,
Sven

It feels like a very convoluted way to implement a safe means of reading an enumeration from a file, but given that that reading incorrect data files is one of the main causes of enums taking on invalid values, I have to say that this is the best solution should we not be allowed the "is" operator or an intrinsic.  If that is the case, then I propose this gets added to TStream!

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

In the meantime, I've extended your AS/IS patch over here <https://bugs.freepascal.org/view.php?id=33603> to create efficient code for x86 platforms, although currently it only does a range check and won't correctly handle enumerations with holes.  If non-contiguous enumerations are going to be allowed, we'll need to design an algorithm that can cover these holes with the smallest number of Boolean conditions.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

At least it hides the gruesome details, cause you only need to call ReadEnum (or ReadRange)... And maybe the code the compiler generates can even be optimized further. :)

Regards,
Sven

I think there is a shorter version, but what is the exact return type of Ord() again?

By the way, what you're reinventing here is type marshalling, for one single type class, and in a very roundabout way.
If that is really what the discussion settled on, it would probably make sense to not tack this onto a particular stream implementation but instead provide a general marshaller for it.

According to this
<https://www.freepascal.org/docs-html/rtl/system/ord.html>, a Longint.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

On x86, when it's a standard enum with a 0 lower bound (most enums are like this) the check can be made as simple as this:

CMP Value, HighValue{ HighValue is calculated at compile-time, while Value is the memory or register that the enum is stored in } JBE @InRange

I'm not sure how well the compiler can generate that from "If (Value >= Ord(Low(TEnum))) and (Value <= Ord(High(TEnum))) then" because it abuses the signed/unsigned convention to check both sides of the domain (i.e. if the value is negative, it is instead treated as a very large positive number that fails the conditions on the jump).

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

J. Gareth Moreton wrote

According to this <https://www.freepascal.org/docs-html/rtl/system/ord.html>, a Longint.

Ah, right.
I checked the source again: that is true only if the argument is an enum. If it is any other ordinal type or Pointer, the result is of the same word size (but possibly different signedness) as the source.

In that case, unlike what was previously said in this thread, this should simply work:

generic function TStreamHelper.ReadEnum<T>(out aEnum: T): Boolean;
begin
  if Read(aEnum, SizeOf(aEnum))  SizeOf(aEnum) then
    Exit(False)
  else begin
    Result := (Ord(aEnum) >= Ord(Low(T))) and (Ord(aEnum) <= Ord(High(T)));
  end;
end;

So this is a comparison of Longints (add a cast to Longword to fix the overflow bug), nothing that should be optimized out. This should also work for subranges in their host types. Unless the optimizer removes the typeconvnodes...

Of course, this needs to be done everywhere a subrange is passed anywhere that is not FPC, so it can't really fix anything.

Ord(aEnum) for invalid enumeration values is undefined ;)

Ondrej

IMO handling holes in enumerations is just a pure waste of time and energy. Delphi documents them as valid and the FPC compiler handles them as valid as well. IMO it is just an unnecessary extra added limitation to disable IS/AS on them. Furthermore they are used mainly for very specific needs, so are very marginal. The effort to make this work just doesn't pay off.

Btw. you get a "project1.lpr(7,10) Warning: Case statement does not
handle all possible cases" in the program below anyway:
program Project1;
type
   THoleEnum = (two=2, four=4);
var
   E: THoleEnum;
begin
   case E of
     two: ;
     four: ;
   end;
end.

What will you do now, disable case-of for enumerations with holes because the compiler cannot handle the holes correctly in the strict FPC POV way?

Ondrej

After considering what Ondrej pointed out about the behaviour of FPC and delphi, I agree that checking lower and upper bounds should be sufficient.

Michael.

Ondrej Pokorny wrote

Ord(aEnum) for invalid enumeration values is undefined ;)

If there was any logic here, it should be, but it's not ;-)

The documentation page specifically mentions Ord as the older syntax to hard casts. Whatever it contains, any enum is always smaller or equal to the largest possible bitsize, which is Longint.

Alternatively, which should be completely equivalent but is not because Ord is Delphi-compatible and subranges are not:

generic function TStreamHelper.ReadEnum<T>(out aEnum: T): Boolean;
var
  tmp: Ord(Low(T))..Ord(High(T)) absolute aEnum;
begin
   if Read(aEnum, SizeOf(aEnum))  SizeOf(aEnum) then
     Exit(False)
   else begin
     Result := (tmp >= Ord(Low(T))) and (tmp <= Ord(High(T)));
   end;
end;

This *could* be optimized out by FPC, but not by a Borland compiler.

It is in fact undefined. The undefined behaviour comes from the fact that an invalid value was stored in the variable and then read again, not from how the read value is used. An example of where you may get different data than wat you put in, even in the absence of any optimisations, is again bitpacked records/arrays.

Jonas

Maybe I'm missing something painfully
obvious here. Can I request an example
where such an invalid value/undefined
behaviour crops up in bitpacked records so
I can see what's going on? Logic dictates
that the number of bits required to store
an enum in that arrangement is equal to
the index of the highest element, although
that's without looking at the source code
and doesn't account for elements that have
assigned negative values.

In the meantime, I'm working on making
"as" and "is" work with ordinal types as
well as enumerations, although currently
some headaches occur if the right-hand
side is larger than the CPU word size
(e.g. Int64 on i386). I'll upload the
patch to the issue once it's working
properly.

Gareth aka. Kit

On Sat 06/07/19 14:19 , "Jonas Maebe"
sent:

On 06/07/2019 14:56, Martok wrote: > Am 06.07.2019 um 09:01 schrieb Ondrej

Pokorny:

>> Ord(aEnum) for invalid enumeration

values is

undefined ;) > > If there was any logic here, it should

be, but

it's not ;-) > > The documentation page specifically

mentions Ord

as the older syntax to hard > casts. Whatever it contains, any enum

is always

behaviour comes from the fact

that an invalid value was stored in the

variable and then read again,

not from how the read value is used. An

example of where you may get

different data than wat you put in, even

in the absence of any

optimisations, is again bitpacked

records/arrays.

I strongly recommend to add these changes at node level, then you do not have to fiddle with the alu sizes of different CPUs.

Most of it is node-level.  Ondrej's method was to use an inlined compilerproc to do the range check, but the parameters are of type SizeInt, which causes problems if the range to check against is of size Int64 or, worse, QWord.  It may be that after some refactoring, the nodes for such an inlined procedure can be inserted directly rather than just inserting a procedure call.

Additionally, I'm putting in an implementation specific to x86 because I can take advantage of its ALU design to do the range check in as few as 2 instructions (a comparison and either a conditional jump or a SETcc instruction).  If the range exceeds the CPU size though, then it falls back to the node-level, platform-agnostic method.

It's probably better to simply show the code for scrutiny once I get it ready, rather than try to explain myself.  Nevertheless, there may be some peephole optimisations that are to be added, but I haven't worked on that in a while due to my overhaul that needs updating so it at least merges with the latest trunk and doesn't undo a load of more recent changes.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

That is correct, but the discussion revolves about storing elements that are larger than that the highest or smaller than the lowest element. In that case, bits gets lost when storing them in bitpacked versions.

That is not necessary. The compiler can do that trick itself (in a CPU-independent way). Before yesterday, it could only do it with unsigned ranges, but since yesterday it can also do it for signed ranges.

It can't yet inline the fpc_is/as_enum because it calls an implementation-only procedure of the system unit, but I'm working on solving that as well.

Jonas

Oh right!  In that case, I can upload the code now... well, after I finish testing everything!  (I'll keep my platform-specific code in a personal branch though in case it doesn't work).

I have seen once source of inefficiency in the compiled assembly language and that's something akin to this (the code is to assign the result of a condition to a Boolean variable - for example, "Result := Value is TEnum;")

  cmpl   $##, %reg
  jnbe   @Lbl
  xorb   %al, %al
  jmp    @Done
@lbl:
  movb   $1, %al
@Done:

(Presumably the xorb %al,%al line was originally movb $0,%al before the post-peephole stage converted it) It might be this way because there was something else in between that was optimised out, but this could be optimised to:

  CMP    $##, %reg
  SETNBE %al

Note that there is one shortcoming that I've discovered while writing a test, and that you can't yet use as/is in a generic function - that is, if you have something akin to this:

generic function<T1, T2>(Value: T1): Boolean;
begin
   Result := T1 is T2;
end;

It will fail, saying that a class type is expected where T2 is specified. I'll open a separate issue for this once the first patches are accepted and cleaned up.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Uploaded the latest patch (AS-IS-enum-07.patch) over here <https://bugs.freepascal.org/view.php?id=33603> and also a test project that evaluates the "is" operator between different ordinal types.

Following the advice of Jonas and Florian, I've removed my x86-specific code for now.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

On an extra note, the assembly language produced is not yet optimal, so it may end up that an x86-specific implementation will be beneficial.  TIsNode contains a virtual method named "DoVariableEnumCheck" that would allow such an extension, coupled with returning "nil" that would defer code generation to "pass_generate_code" (allowing this, but not overriding "pass_generate_code", will trigger an internal error, because the 'is' node normally never allows it to be called).  Nevertheless, even if that is a no-go, it's making me a little excited to see if I can find new peephole optimisations to implement.  But until my old x86_64 overhaul is accepted, rejected or reworked (at least to make it successfully merge), I can't really make any new additions yet.

Gareth aka. Kit

P.S. One optimisation I would like to suggest across all platforms, if at all possible, is to defer register allocations until after the first peephole optimisation stage, because I see sequences like this on i386 when working with Int64's (this particular sequence sign-extends from LongInt to Int64):

mov %reg1,%reg2
sar $31,%reg2

This fills %reg2 with all 0s or all 1s depending on if the sign bit is set in %reg1.  However, if the registers are still virtual, it may be possible to evaluate nearby code and request a preference for %eax for %reg1 and %edx for %reg2 (certain operations like "mul", "div" or function parameters require the use of %eax and %edx specifically) - that way, the sequence can be collapsed into a single call (and a single byte in size) to "cdq".

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Il 07/07/2019 07:33, J. Gareth Moreton ha scritto:

Sorry for jumping in without having followed the full discussion, but I wonder if "as" and "is" are the best constructs to implement range checking. Wouldn't "in" be much more appropriate?

Sort of:

if value in enumvar then DoThat else IgnoreIt;

Giuliano

The IN operator
1.) expects a value on the right side, not a type. 2.) can be overloaded:
https://www.freepascal.org/docs-html/ref/refse104.html#x213-23500015.6 . IS/AS cannot.

Ondrej

I though I convinced you not to disable the feature on enums with holes even in OBJFPC mode...

Ondrej

I disabled it after Pierre pointed out
that you couldn't use Pred or Succ with
them, so wanted to be consistent,
especially as programming them to return
the expected value of False of a value
falls within a hole is not simple. Still,
it's easy enough to re-enable.

Gareth aka. Kit

On Mon 08/07/19 12:51 , Ondrej Pokorny
sent:

On 08.07.2019 02:37, J. Gareth Moreton

Uploaded the latest patch (AS-IS-enum-

07.patch) over here [1] and also a

test project that evaluates the "is"

operator between different ordinal

types. I though I convinced you not to disable

the feature on enums with holes

There has been a much bigger discussion about it than the one post from Pierre.

Ondrej

Apologies.  I've been getting lost and confused in all the details and am not even sure what people want any more.  What is the consensus from the administrators?

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

I need to know exactly what's been decided upon.  No arguments, no debates, no objections.  What has been accepted as "this is what will be implemented after all parties have been heard"? Because I can't put myself into a meltdown while my neck is being wrung for not doing what people wanted.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Some non-technical remarks regarding code formatting (I know the compiler does currently not use a consistent style, but there is one that is considered the de facto one (I should find the time to write that down in the Wiki some time -.-) and new code should be added following this):
- no spaces between operators and symbols, both in assignments and expressions as well as parameter declarations
- local variables are written in lower case with the declarations as "name1,name2,name3 : type"
- types are written in lower case
- method names are lower case with '_' as separator between words (so your DoVariableEnumCheck would become do_variable_enum_check)

And one very important technical remark:
*No* "as" (or "is") inside the compiler. Use hard casts as you already checked for the def type

Regards,
Sven

Sven Barth schrieb am Di., 9. Juli 2019,
07:41:

And I'm rather conflicted regarding the usage of "with". But Florian and/or Jonas should comment whether we want new ones in the compiler or not.

Regards,
Sven

I don't recall using "as" or "is" directly, but if I've done so, I humbly apologise and will fix that right away.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Apologies - I see now... I used them with "with" without even thinking about it!  I've made the changes as requested. Admittedly it's quite hard for me to remove spaces from either side of operators because that's something I learnt from my early days in Turbo Pascal and to not have spaces (and all types and variables being lowercase) feels very C-like.  But hey, rules are rules... or guidelines!

Is the new AS-IS-enum-08 patch any better?

Gareth aka. Kit

J. Gareth Moreton schrieb am Di., 9. Juli 2019,
08:20:

I know the feeling. But when in Rome...

Is the new AS-IS-enum-08 patch any better?

Better (though are still some " = " around :P ), but it seems you missed tasnode.pass_1?
Also the typenames (TOrdDef, TEnumDef) should be lowercase as well as the local variables.

Regards,
Sven

Hopefully we'll get there in the end!  I've updated what I can in the patch file directly, so hopefully all is well now.

Some of the variable names, like "ProcName", were already named as such.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

Should I modify the patch to allow enumerations with holes with "is" and "as"?  Note that these operators will return True/not raise an error if a value falls within a hole but is otherwise between the lowest and highest elements.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

J. Gareth Moreton wrote

Should I modify the patch to allow enumerations with holes with "is" and "as"?  Note that these operators will return True/not raise an error if a value falls within a hole but is otherwise between the lowest and highest elements.

IMO, yes.

Michael.

Done!

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

J. Gareth Moreton wrote

Done!

Thank you for your work!

BTW, your note in the issue report is very misleading: https://bugs.freepascal.org/view.php?id=33603#c117162.

The idea behind "is" and "as" is that they are the sole exception to

the rule and will return False and raise an error respectively if the enum is out of bounds.

IS/AS are not an exception of the rule - after a long discussion we settled down that if IS/AS is applied on a value of the same enumeration type:

var
   MyEnumValue: TEnumValue;
begin
   if MyEnumValue is TEnumValue then
// ...
end;

the compiler will expect that the result is always true. (It doesn't have to check for invalid values.)

When I read the following sentence:

If the value falls within a hole, then it will pass - currently only

in Delphi mode though. FPC mode won't allow it.

I think you mixed up "normal enums" and "enums with holes" and so the note in the issue report doesn't make any sense.

----

The conclusion is:
1.) IS/AS works reliably only on valid values on the left side of the operator. (Like any other function/intrinsic/operator in FPC.) 2.) If an invalid value is to be expected, IS/AS must be executed on an ordinal value (Integer, SmallInt, ...) before assigning it to the enumeration value. It cannot be executed on the resulting enumeration value to check its validity.
3.) IS/AS can be used also on "enumerations with holes". In this respect only the low/high bounds are checked, not the holes inside the valid range. 4.) IS/AS can be also used on ordinal types:
 - 4a.) (MyInt64Value is Integer) is a reasonable check if MyInt64Value is in Integer range.
 - 4b.) (MyIntegerValue is Int64) is not reasonable (Integer falls always into Int64 range) and will always return True and can/will be optimized by the compiler to return a True constant.

Ondrej

Who is "we" in this case? I was under the impression that is/as could be used to check if the enumeration was valid or not, as a means to check if the enueration contained a valid value or not so as to avoid problems with case blocks raising access violations. I guess we've hit disagreements again.

Gareth

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

There is no "disagreement". just doubt as to what the right approach is.

If the compiler assumes that an enumerated always contains a valid value, then the logical consequence is that the above "is" always evaluates to true. In that case it makes sense to downright forbid the construct, as it only makes sense if MyEnumValue is another ordinal type.

The case of bitpacked values and variant records shows that the language allows for cases where this assumption is maybe not correct. From this point of view, the above construct is a valid check and should not be optimized away.

It now depends on your point of view what road to take.

Michael.

Well, the patch ("AS-IS-enum-10.patch") was made on the assumption that "MyEnum is TMyEnum" will do a range check and return False if it's out of bounds.

Outside of typecasting, there isn't a reliable way to check if an enum contains a valid value, which leads to the problem of reading an enum from an external source that may be tampered or corrupted.  I had the impression that "as" and "is" would be a reliable sanity check to see if the enum is in bounds.

For my personal point of view, I would like these operators to be the exception to the rule of the compiler assuming an enum always contains a valid value, and above all, the documentation improved for "case" that advises the programmer to use the operators if they are using enumerations and there's a chance the input may be invalid.  Whatever decision is made though, I do think there should be a warning in the documentation and a suggested workaround just so we don't keep getting bug reports.

I'm not on the core team though, so the final decision ultimately doesn't rest on me, but for what it's worth, I would like "MyEnum is TMyEnum" to not assume the answer is True and to actually perform an explicit check.  However, there is an argument to be made for both approaches.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

J. Gareth Moreton wrote

Outside of typecasting, there isn't a reliable way to check if an enum contains a valid value

Typecasting is not a reliable way. There is no and cannot be a reliable way, period.

For my personal point of view, I would like these operators to be the exception to the rule of the compiler assuming an enum always contains a valid value,

This is the crux of the whole issue. Type systems do not and cannot work this way. At its heart, this discussion is not about compiler assumptions, optimisations, exploiting undefined behaviour etc.

It's that if a language has a type system, then by definition all reasoning over programs written in this language must follow the rules of this type system. A fundamental part of this is that a variable of type X can store all values that are valid for type X. Not more, and not less.

This is true for FPC, Delphi, C, C++, C#, Java, ADA any every other programming language in existence that has a type system. Otherwise you do not have a type system, but at most some ad hoc behaviours that vaguely resemble a type system. You can never deviate from this, or you end up with an unmaintainable and unpredictable mess (regardless of how well you know the basic rules of the language, and not just when you are used to type system with different rules).

In Delphi's type system (and in C, C++, and C#), the valid values for an enum are always all values that fit within its storage size, and that storage size is fixed.

In FPC's type system (and in Ada and Java), the valid values for an an enum are only the values between the lowest and the highest declared enum element (in Java it's even more strict, in that when you have "enums with holes", the holes are also invalid), regardless of the storage size. Moreover, this storage size can vary depending on whether or not the item is in bitpacked storage. That definition of enums is also what allows us to bitpack these types in the first place, since otherwise we would not be able to store all valid values for enums in their bitpacked versions.

Jonas

J. Gareth Moreton wrote

For my personal point of view, I would like these operators to be the exception to the rule of the compiler assuming an enum always contains a valid value,

As much as I would like to agree with you (and yes I think such a check, if possible, would be desirable - even if it meant to ignore certain language design principles), it is indeed not possible. Maybe it would be today (as in today's fpc implementation), maybe not. I do not know.

But it would not be future safe.

Lets say "fpc breaks the rule for "is/as" and only for those".

   FooEnum := SomeApi();
   if not (FooEnum is TFooEnum) then FooEnum := low(TFooEnum);

Now we do *not* know what the optimizer does in the first line. And that is the problem.
On some platform (that we have no clue of today), the compiler may store the value bitpacked into a temp var/register. The storage is shared with other values.
If the result of "SomeApi()" is out of bounds, it might overwrite one of the other values stored in that var/register.
The "is" can only check the part of the storage actually meant to be used by the enum. The change to the other value goes unnoticed.

So in such a future case, the is may do what you expect. A later "case FooEnum" will be ok, as FooEnum is "forced" to be valid. But your program still crashes, due to the other var being modified unexpectedly.

Yes that may sound far fetched. But maybe some embedded system, with scarce resources requires such behaviour?

----------------
Maybe a possible solution would be based on:

Jonas Maebe wrote

In Delphi's type system (and in C, C++, and C#), the valid values for an enum are always all values that fit within its storage size, and that storage size is fixed.

Create such an enum type in fpc too (as a new type / assignment compatible with the current enum)

   {modeswitch StorageSizeValidEnum}
   type TFullFooEnum = (a,b,c);

would treat all values of the (unpacked) storage as valid. As a consequence:
 - it can not be packed / it would keep its full size  - many optimizations can not be applied

But for such a type, that has explicitly been defined as having all the other values, you could use "is".
Of course not straight forward. Because now  255 is a valid value for that enum, so "is" would return true. But with:

   {modeswitch StorageSizeValidEnum on}
   type TFullFooEnum = (a,b,c);
   {modeswitch StorageSizeValidEnum off}
   type TFooEnum = (a,b,c);
this would work:
   if  FullFooEnum is TFooEnum then

Since this is only/mainly used for "external" data, the extra type def would not be to much overhead.

Of course, it is merely syntactical sugar, since it is identical too
   type TFullFooEnum = integer; //or whatever represents the storage size
   type TFooEnum = (a,b,c);

It only allows to have named values for that "integer"

Okay, okay.  So what do we do to stop all these bug reports and help programmers who get caught out by case blocks raising access violations?  Are you effectively saying "don't ever use enumerations for external files and network signals" etc?

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

J. Gareth Moreton wrote

Okay, okay.  So what do we do to stop all these bug reports and help programmers who get caught out by case blocks raising access violations?  Are you effectively saying "don't ever use enumerations for external files and network signals" etc?

Declare your enumation types so that the lowest and highest valid value comprise the lowest and highest value representable by its storage:

{$mode delphi}
{$z1}
type
  tmyenum = (ea, eb, ec, emax = 255);

That behaves the same both in FPC and in Delphi.

Jonas

I see your point.  There seems to be some argument because it effectively bans the use of enumerations in external data (e.g. a file, network packet or third-party API) if there's a chance that the value is corrupted or has been tampered with, because sometimes allowing the program to crash is not acceptable.  Though misinterpreted, Michael said this to me when I mentioned to the effect of "in that case, you could only use Integers etc. for external data":

...

With this sentence you forbid storing or communicating enumerated values in any way: file, database, over network. It can be used only in a computer program and never
leave the context of the running program under any form. Because as soon as it is somehow communicated, there is a chance it becomes invalid in return communication.

Additionally you must then also abolish typecasting to an enumerated (or pointers to enumerated values), since that also can be a source of invalid values for an enumerated.

Or are you realy advocating we write code such as

Case MyInteger of
   0 : MyEnum:=ValueOrd0
   1 : MyEnum:=ValueOrd2;
else
   someError
end;
etc, whenever an integer must be changed to an enumerated ?

IMHO you would reduce the usabilty of enumerateds to almost zero by doing so.

...

Here lies the problem - if you have a enumeration stored in a byte field, say, in a file, and there's a chance it's been corrupted, there's no reliable way to detect that, since even typecasting may not be reliable, and once again I'm implying that enums are unsafe to use, but right now, what else can one do?

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

This is a completely pointless definition, since for effective use you still need to add code to check that the value is a named one.

Michael.

That is exactly what this thread is about: the ability to manually check whether an enumeration value is one of the valid (named) values.

Jonas

Well yes. Reading from a file is like a typecast.

TEnum(SomeInt) implies that you have done the validity check *before* this. Because by using the type you declare that the result is valid.

Of course reading from a file/api you can not do the test before....

But if you want the test afterwards, you must change the entire definition of the type. Changing it for just one statement (the "is") can not work for technical reasons (see my previous post). You do not know what other side-effects you may have done in the statement that typecasted an unchecked/invalid value. If you do not know the list of possible side effects, then you can not test for all of them.

I would guess this would work

{$mode delphi}
{$z1}
type
  tmyFileEnum = (ea, eb, ec, emax = 255);
  tmyenum = (ea, eb, ec);
read(file, AFileEnum);
if AFileEnum is TMyEnum
then AMyEnum := TMyEnum(AFileEnum)
else DoError;

I know.

I simply do not want to change a type definition just to keep the compiler happy.

I want to define a type as dictated by my use case: tmyenum = (ea, eb, ec)

and then some easy means to check if an ordinal value fits. Hence the Is/as.

I yesterday checked a database project I have. I counted 25 enumerateds which are stored in the database. Without fail, for each type there is somewhere code in a type helper:

if (MyDatabaseValue>=0) and (MyDatabaseValue<=Ord(High(TMyENum))) then
   MyENumValue:=TEnum(MyDatabaseValue))
else
   EConvertError.Create('Invalid value for TMyEnum : %d',[MyDatabaseValue])

If I can replace this with a simple

   MyEnumValue:=MyDatabaseValue as TMyEnumValue

It makes life a lot easier. So this construct has my support.

That the case where you would write

if MyEnumValue is TMyEnumValue then

can 'by default' be optimized away because as far as the compiler is concerned this is a tautology, is for me a perfectly acceptable and defendable point of view. Even to such a degree that the compiler would simply reject your code by not allowing is/at on a variable of the type itself. Also defendable.

The check on a valid value using is/as should be done before the enum variable is used/assigned.

I just want to make the check easier.

Michael.

I do exactly the same - check the low/high bounds in a type helper :) Yes, and I am tired of typing it as well :)

I agree.

Even to such a degree that the compiler would simply reject your code by not allowing is/at on a variable of the type itself. Also defendable.

No - not this one. Don't forget about e.g. generics:
function Test<T>(MyValue: T): Boolean;
begin
   Result := MyValue is TMyEnum;
end;
would compile for all enumeration types but not for MyValue:TMyEnum.
That doesn't make sense.

Better to have a compiler hint/warning when such a construct is found and optimized. This perfectly corresponds with the fact that also constructs like "SmallIntValue is Integer" can be optimized away but must be allowed.

Ondrej

Don't forget about my second suggestion: add internal range checking for enumeration values on specific operations:
https://lists.freepascal.org/pipermail/fpc-devel/2019-July/041499.html

The "BUT (big BUT)" section in the 2nd half of the email. I copy it here with one small update:

BUT (big BUT): I suggest to do an enumeration/subrange validity check on the compiler side when range checking is used. This is similar to the current fpc_check_object intrinsic.

When range checking is used, this check should be injected (at least) before every case-of and the new is/as operators if they are replaced with true. When range checking is disabled, the check should not be for range checking:

1.) case MyEnumValue of // <<< here an enumeration validity check should be injected
2.) IsValid := SomeValue is TEnumValue; //  <<< here an enumeration validity check should be injected if SomeValue is an enumeration type. (It doesn't make sense for normal ordinal values like Integer because they are valid in the whole stored range.)
That would catch the
IsValid := MyEnumValue is TEnumValue;
check - it would be replaced by a TRUE constant but there would still be a range check injected that would raise an error in case MyEnumValueis not valid.

IMO such a solution is acceptable for both sides:
1.) Jonas' point: The clarity of the language will stay untouched because the new validity check will be only informative and used only for debugging without any assurance of correctness. (You see the difference: MyEnumValue is TEnumValue will raise an exception for an invalid value and not return false! False will be returned only for (MyInt is TEnumValue) if MyInt is ot of the range of TEnumValue.)

2.) My/J.Gareth's point: We will get an information about invalid enum values (range check error) instead of "undefined behavior" when debugging - and we
will be able to find code where invalid enum values can occur and fix the code where these values were assigned. Although this information may not be (theoretically) absolutely accurate (the same as fpc_check_object is not absolutely accurate), it will still be pretty accurate and it will be a big help during the debugging.

Ondrej

Fair enough.
Like I said: The point is defendable. So is your point of view.

Personally I am OK with either point of view.
I am just interested in the ordinal, to make an easier check possible.

I think all sides have now been reviewed to the point of boring or annoying each other almost to death, and we finally need to decide on whether the patch is applied, and if so, which parts of it.

Michael.

PM Ondrej Pokorny wrote

I do exactly the same - check the low/high bounds in a type helper :) Yes, and I am tired of typing it as well :)

You can pretty easily write a generic function that will work on pretty much any enum for this, BTW:

program Example;
{$mode Delphi}
uses TypInfo, SysUtils;

function TryConvert<T>(const Value: SizeInt): T; inline; begin

  if (Value >= SizeInt(Low(T))) and (Value <= SizeInt(High(T))) then
    Result := T(Value)
  else
    raise EConvertError.Create(
      Format(
        'Invalid value for %s : %d', [
          PTypeInfo(TypeInfo(T))^.Name,
          Value
        ]
      )
    );

end;

type
  Letter = (A, B, C);
begin
  WriteLn(TryConvert<Letter>(0));
  WriteLn(TryConvert<Letter>(1));
  WriteLn(TryConvert<Letter>(2));
  // Exception!
  WriteLn(TryConvert<Letter>(3));
end.

The exception printing "EConvertError: Invalid value for Letter : 3", of course.

No doubt, but this will lead to a bloated binary. I want less code (both source and assembler), not more.

Michael.

PM Michael Van Canneyt wrote

No doubt, but this will lead to a bloated binary. I want less code (both source and assembler), not more.

Well, it would be one instantiation per unique type it was used on.

So if you had five hand-written helper functions for different types, and replaced all of them with the use of a single generic function, you'd still end up with just five functions worth of assembler code (i.e. the same amount you had to begin with.)

I know. See below.

Yes, I know. But the goal is to have 'is/as'. Then there is at best only 1 helper function for all enumerateds.

I have a Delphi project that uses really lots of generics. The decrease in compilation speed and increase in binary size is truly noticeable.

<aside>
In general, I strive to minimalize the use of generics or avoid there are little use cases that really justify the use of generics. But that's a highly personal opinion.
</aside>

Michael.

PM Michael Van Canneyt wrote

Yes, I know. But the goal is to have 'is/as'. Then there is at best only 1 helper function for all enumerateds.

Oh yeah, I wasn't really suggesting a generic function as a *solution* to the problem versus "is" and "as". I was just pointing it out as a concept that works in general and can be used right now (for things other than enums as well).

Side note: if this was done 100% consistently (and it does make sense!), the PACKENUM directive would be completely useless. There is no point in being able to specify the storage size of an enum when it can be and is # and C++ do the same with explicitly sized enumerations. Who would I have to bribe to get that in FPC?

{$mode objfpc}
type
  tmyenum : Byte = (ea, eb, ec);

That would solve literally all problems we ever discussed:
- the "scoped constant" concept is obvious when used
- for bitpacking purposes, the bitsize is that of the explicit ordinal
- therefore, any same-size cast (API return, file io, network io) is valid
- {$Mode TP} and {$Mode Delphi} would treat all enum declarations as sized enumerations of whatever $Z is set to at declaration time, fixing the "same syntax but invisibly different result" portability issue
- both type concepts would be consistently handled in the same type system
- whenever that syntax is not used, all existing optimizations are valid
- when it is used, others may be chosen
- the case label checks would also catch any problems we don't yet see

Anything else (including the intrinsics this is about) is just a hacky workaround to the difference Jonas summarised above, IMHO.

Ben Grasset schrieb am Sa., 13. Juli 2019, 21:52:

Not necessarily. If you have two units that don't know about each other that specialize the function with the same enum then you'd have two specializations already.

This would need either a WPO pass or COMDAT support. Both are on my ToDo list, especially as the former is needed if the output format does not support the idea of COMDATs (e.g. PE/COFF and ELF do).

Regards,
Sven

Just a thought that I'd chuck in on this... I did wonder how practical it would be to store checksums of all compiled procedures and the like and to look for collisions.  When studying how the compiler is built, I noticed a lot of the internal methods, due to conditional defines, would compile into identical code, so merging them would reduce the binary size if the linker is able to strip out procedures that are never called.  Granted, I haven't done research on how Free Pascal does Whole Program Optimisation yet, especially as it requiring a separate compilation pass has always put me off.  I would have thought that the linker should be able to handle it right there and then because it has all the information it needs.

Gareth aka. Kit

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

That would be a possible approach to use if the output format does not support combining equal sections. PE/COFF's COMDAT sections do have a flag that says that they need to have equal content (there are also other possibilities). Don't know right now whether ELF does.

Regards,
Sven

in a bitpacked array/record, i.e., when the programmer explicitly instructs the compiler to ignore it. In other cases, the requested storage size is reserved.

However, while storage size and valid values are related (you have to have enough storage size to store the valid values), the fact that a particular type can store a particular bitpattern does not mean that this bitpattern is a valid value for the type. Just take an ansistring: you can put any bitpattern in it that fits in a PtrSInt, yet most of them are invalid and will result in undefined behaviour.

The most common cases to specify a storage size would be alignment, and the ability to add more valid values in the future without having to change the layout of a data structure.

It would solve nothing, and rather demonstrates the disconnect that apparently still exists in this discussion: the ability to bitpack enums (and hence reduce their storage size) is a mere side effect of the fact that their valid range only goes from the lowest to the highest declared value. You could reserve 4KB for an enum with as only valid value "enumX" and it would not change a thing as far as the valid values or the compiler behaviour are concerned.

The issue with a modifier or directive that would change the definition of which values are vali for an enumerations with this modifier/within this directive, is (besides the inconsistent low/high and range checking behaviour from Delphi this would require, especially when subranges also enter the picture) explained in the post I linked near the beginning of the thread:
https://forum.lazarus.freepascal.org/index.php/topic,45507.msg322059.html#msg322059

Jonas

Optimising linkers that do this exist. However, it's tricky to do it in a safe way because programs may compare addresses of functions to determine what to do. If those addresses become equal, program behaviour can change. E.g.

procedure test1;
begin
end;
procedure test2;
begin
end;
procedure doit(proc: tprocedure);
begin
  if  then
    writeln('test1')
  else
    writeln('test2');
end;
begin
  doit(@test1);
  doit(@test2);
end.

Jonas

Oh please, don't troll. I know that you know that "is reserved" and "is used" are not the same thing.

The most common cases to specify a storage size would be alignment, and the ability to add more valid values in the future without having to change the layout of a data structure.

... the only reasonable application of which is some kind of IO or an ABI that shouldn't change, for which enums must not be used directly in the first place.

No, you couldn't. The largest ordinal type is QWORD.

The issue with a modifier or directive that would change the definition of which values are valid for an enumerations with this modifier/within this directive, is (besides the inconsistent low/high and range checking behaviour from Delphi this would require,

This is easy, enums already have separate min/max and a basedef. Make the basedef a type that can hold an orddef, modify calcsavesize and packedbitsize accordingly, done.
Low/High will continue to provide the named range. Arrays of such a type should not be allowed (as for enums with jumps right now) (* although they could be without surprises, Array[tmyenum] would simply be Array[Byte] *).

On Subranges, that's also simple and obvious. That's the beauty of it, explicit sizes would be explicit. They are not "inherited":

  tmyenum : Byte = (ea, eb, ec);
  tsubenum = eb..ec;
     //^ basedef=tmyenum, no explicit type, "fpc-style" enum,
     //  inherited symtable with ordinals $01 and $02
  tsubwenum: Word = eb..ec;
     //^ basedef=u16inttype, new symtable with ordinals $0001 and $0002
While the first example looks inconsistent, it is the same as what we already have:
{$PACKENUM 1}
  tmyenum = (ea, eb, ec);
{$PACKENUM DEFAULT}
  tsubenum = eb..ec;
tsubenum will be 4-bytes, tmyenum is 1 byte, same as with the new syntax.
tsubenum remains assignment compatible to tmyenum.

In contrast tmyenum.eb and tsubwenum.eb are not compatible, as the obviously cannot be, since they have different _explicit_ sizes. I can't think of a usecase for the second declaration in the first place, but getting correct behaviour as a side effect is always a good sign.

the post I linked near the beginning of the thread: https://forum.lazarus.freepascal.org/index.php/topic,45507.msg322059.html#msg322059

As I wrote in the last message, those points would be fully addressed by this proposal. Choosing at declaration time is the only valid solution, all modes will behave exactly the same once the type is defined.

Best,

Martok

Please refrain from throwing around insults.

I know that you know that "is reserved" and "is used" are not the same thing.

If you want to nitpick, the compiler will perform 1/2/4 byte writes for enums of those sizes, so the full reserved data is in fact used/initialised. Again: the only relevant part in this discussion is the valid values. The reserved/used/accessed/written/... storage size is unrelated to that to the extent explained in my previous message.

This inconsistency would mean that we at least need two versions of defutils.getrange() in the compiler: one for (some) range checks (using the named range), and one for other reasoning (which must use the basedef range). There may be other such places that need modifications.

Arrays of such a type should not be allowed (as for enums with jumps right now) (* although they could be without surprises, Array[tmyenum] would simply be Array[Byte] *).

The whole point of a directive/modifier would be to get Delphi-compatible enums, not to add a third variant.

On Subranges, that's also simple and obvious.

I meant that Delphi's range checking for enums/subranges doesn't make any sense due to the inconsistency between the "valid" and "declared" range.

E.g., take this Delphi program (tested with Kylix 3; YMMV with modern Delphi versions, to which I don't have access)

{$z1}
type
  tenum = (ea, eb, ec, ed);
  tsubenum = eb..ed;
  tsubsubenum = ec..ed;
var
  enum, enum2: tenum;
  subenum: tsubenum;
  subsubenum: tsubsubenum;
  arr: array[tenum] of byte;
begin
{$r+}
  // no problem
  enum:=tenum(255);
  // no problem
  enum2:=enum;
  // segmentation fault
  arr[enum]:=2;
  // range error
  subenum:=enum;
  // no problem
  subenum:=tsubenum(255);
  // no problem
  enum:=subenum;
  // segmentation fault
  arr[subenum]:=2;
  // range error
  subsubenum:=subenum;
  // no problem
  subsubenum:=tsubsubenum(255);
  // no problem (!)
  subenum:=subsubenum;
  // no problem
  enum:=subsubenum;
  // segmentation fault
  arr[subsubenum]:=2;
end.

This is both 100% inconsistent and 100% logical.

It is logical because according to the (Delphi and FPC) type system rules tsubsubenum <= tsubenum <= tenum, and hence every tsubsubenum by definition is a valid subenum and a valid tenum, every tsubenum is a valid tenum (and every valid t(sub)(sub)enum is a valid t(sub)(sub)enum). As a result, you don't need any range checks when converting them in that direction (an array indexation is also a type conversion to the index type, hence the segmentation fault rather than range error). However, the other way round does not necessarily hold, and hence there are range checks when going in the other direction.

On the other hand, you have this whole inconsistency where 255 is both a valid and an invalid value. So when a range check happens to be inserted, you get a range check error, and when it isn't, you don't.

The only way to implement consistent range checking in this scenario is by having range checks for every single enumeration type conversion, even between the same type. Although then you get the curious situation where 255 is at the same time both valid and triggering range check errors all over the place as soon as you dare to use it. Plus you get a whole bunch of unnecessary range check operations in code unless you add a separate optimisation pass to remove them.

I completely understand the practical use case of this type of enums, but from a type/correctness checking point of view it's utter nonsense.

As described in that post, the issue is that the default FPC units declare various enumeration types without any specific modifiers. Even if all default FPC units would be changed to Delphi-compatible enums, then you would still get issues when mixing and matching units declared with differently declared enumeration types, since there is no obvious way to know how a particular enumeration type has been declared.

Jonas

Nihao,

Not to mention, that this is the job of the {$PACKENUM} directive, ensuring an enumeration type reserves a minimum amount of space.

That was exactly my point!

The storage size is unrelated to the "used"/"valid" size, therefore there is little point in being able to set it.
I can tell the compiler "this type is 4 bytes wide", but it will still only ever consider the lowest byte if that's how many elements there are.

This inconsistency would mean that we at least need two versions of defutils.getrange() in the compiler: one for (some) range checks (using the named range), and one for other reasoning (which must use the basedef range). There may be other such places that need modifications.

Interestingly, that's not the case, I checked.
There are like 2 major places that need adjustment, everything else already correctly uses getrange for the type range and min/max for label range. It almost looks as if most of the compiler was written assuming these two are not the same.

The JVM class generator is the only real challenge. These days, one could probably take a look at .Net Core to see how a managed language does this.

Where do you see a third variant? If you think this is anything but a "Hejlsberg-Enum", I seriously failed to explain it.

The proposed syntax would be a shorthand for what Delphi does when you write
  {$Zn} tenum = (ea,eb,ec,ed);
With n:=sizeof(basetype).

Nothing else, nothing more.

I meant that Delphi's range checking for enums/subranges doesn't make any sense due to the inconsistency between the "valid" and "declared" range.

The thing about $R and Delphi: a) it is purely ancillary and not part of the type system. A *lot* of stuff is not rangechecked has not a lot to do with the nesting of type ranges.
b) only pred/succ, literal assignment without hard cast and assignment from larger to smaller type are guaranteed to check if a subrange type is exceeded by an action. Nothing else does - anything(!!) that fits in the same storage is a valid value.

Both of that is documented behaviour, and has been for decades.

One might argue that it is 100% consistent as well. Compile in {$R-} (remember, in TP and Delphi it's a debugging aid, not a
thing-you-have-to-do-to-get-somehwat-safe-results like in FPC), and only the out of bounds array accesses remain. And those are 100% equivalent to our old friend, the unchecked jumptable access.

Except, you know, looking at the source?

That's like arguing that there is no way to know if a record is packed or not. Of course there is, it's a keyword right in the declaration.

Or, actually, more like asking what the calling convention of a function could be. Usually you don't need to know, but when you do, it's either a keyword at declaration site, or a {$CALLING} setting is in effect. No 's why I say the decision must be clear at declaration point, not for every access (as was previously suggested).

Best,
Sebastian

On Sat, Jul 13, 2019 at 9:02 PM Sven Barth via fpc-devel <

Surely that only applies to what winds up in the PPUs for the units in question, if anything, though? As opposed to the object files and the final executable. Like, I don't see how it could possibly be the case that if two / three / four / e.t.c units all use Generics.Collections, and each one contains an instance of something like:

var IntList: TList<LongInt>

that this means the resulting binary contains two / three / four / e.t.c separate complete instantiations of TList for LongInt. You'd wind up with executables in the literal hundreds-of-megabytes range if that were so, even with optimizations / symbol stripping and so on activated.

Ben Grasset schrieb am Mo., 15. Juli 2019, 22:57:

That is exactly what is happening if you have a specialization in multiple units that don't know about each other.

Regards,
Sven

Indeed. And this is a very common scenario.

For this reason, I explicitly declare all specializations I need in a separate unit. To make sure there is only 1 of them for each type.

Michael.

On Mon, Jul 15, 2019 at 5:23 PM Sven Barth via fpc-devel <

At what point are they being removed so that the executable is not comically large, then? Is it on the FPC side, or the linker side? Or a bit of both perhaps?

As far as I know, they are not. I believe that when specializing, the compiler checks if an identical specialization is in scope: if so, it uses that. If not, a duplicate is made.

As far as I know, Delphi behaves the same.

Michael.

I was aware of the "identical specialization" check (or at least, I assumed it worked that way.)

It still seems to me though that while heavy use of generics is certainly reflected in PPU size (for example, on x86_64 Windows, the release mode PPU of Generics.Collections.pas with my build of FPC is slightly over 30 MB) the PPU size does not really correspond to object file size or overall binary size (which again might be thanks to some stage of FPC itself, or the linker in the case of external ones, or both.)

I suppose FPC does probably diverge from Delphi behavior at some point there too, as Delphi uses only DCUs, with no separate ".O"s.

Michael Van Canneyt schrieb am Mo., 15. Juli 2019, 23:50:

Delphi changed this in one of the newer versions. That's also when the behavior of class variables in generics changed.

Regards,
Sven

Ben Grasset schrieb am Di., 16. Juli 2019, 00:13:

The *declaration* of a generic is reflected in the size of the PPU as the token stream is stored there. While *specializations* are reflected in the compiled object code, aka the .o file.

Regards,
Sven

Ben Grasset schrieb am Mo., 15. Juli 2019, 23:48:

They are not removed. At least not in the way you think:

Declare a type" TTest = specialize TFPGList<LongInt>;" in two different units (here ugentest1 and ugentest2) and then have the following program:

=== code begin ===

program tgentest;
{$mode objfpc}
uses
  ugentest1, ugentest2;
begin
  // TFPGList.Assign is a non-virtual method
  Writeln(HexStr(@ugentest1.TTest.Assign));
  Writeln(HexStr(@ugentest2.TTest.Assign));
end.

=== code end ===

The output will then be two different addresses, thus showing that the code had been generated twice.

However the compiler/linker is good at leaving out unused code and in this case only the two Assign specializations are kept as their addresses are taken, everything else is discarded. If you'd change one of the two to another non-virtual method then Assign of the one specialization and the other method of the other specialization would be kept.

If you use virtual methods inside generics then things are less likely to be removed.

Regards,
Sven

For the record:

The above will not work in delphi for at least 2 reasons:
1. Simple fuctions are not supported. Can be worked around by making it part of a record.
2. The Low(T) does not work, as there is no way to tell Delphi that T must be an enum.

Michael.

On Tue, Jul 16, 2019 at 5:28 AM Sven Barth via fpc-devel <

Ah, I understand now.

I guess my point overall was that, in my experience at least, while there's indeed probably room for improvement in that area via something like WPO / COMDAT support / e.t.c. as you'd suggested, the overall effect of generics in FPC on binary size is not *unreasonable*.

I use Delphi only semi-regularly, but it does seem to be somewhat more of an actual issue there (as frankly binary size in recent Delphis is in general, with generics or not, when compared to FPC.)

I suspect this is also in part due to the liberal and generous use of extended RTTI.

Michael.

Michael Van Canneyt schrieb am Di., 16. Juli 2019, 16:31:

Correct. We'll have to see how that works out with FPC...

Regards,
Sven

AM Michael Van Canneyt wrote

2. The Low(T) does not work, as there is no way to tell Delphi that T must be an enum.

Hmm, I guess it makes sense Delphi wouldn't allow the "Low", based on other Delphi generics behavior (more like C#, whereas FPC is more like C++.)

I only realized recently that you can't even actually use "arithmetic" operators on Delphi generics at all, E.G, the following is valid FPC code, but not valid Delphi code:

program Example;
{$mode Delphi}
type
  TComparer<T> = record
  public
    class function Min(const A, B: T): T; static; inline;
    class function Max(const A, B: T): T; static; inline;
  end;
  class function TComparer<T>.Min(const A, B: T): T;
  begin
    if A < B then
      Result := A
    else
      Result := B;
  end;
  class function TComparer<T>.Max(const A, B: T): T;
  begin
    if A > B then
      Result := A
    else
      Result := B;
  end;
begin
  WriteLn(TComparer<Single>.Min(2.0, 4.0));
  WriteLn(TComparer<Single>.Max(2.0, 4.0));
end.

Michael Van Canneyt wrote

I think all sides have now been reviewed to the point of boring or annoying each other almost to death, and we finally need to decide on whether the patch is applied, and if so, which parts of it.

That would be great. I have been waiting for some kind of decision or at least some statement from the compiler team for more than a year. Are there any news regarding the decision progress?

Ondrej

It has not been discussed on core.

As far as I am concerned, the patch can be applied. But since I don't manage the compiler, I have little voice in the process.

I will raise the subject on core.

Michael.