Hi everyone,

So it looks like this new warning has appeared as part of the data flow analysis of -O4.  The thing is, I personally have a problem with this being a warning, because there's nothing inherently wrong with not covering every case branch or omitting an else block (especially if one isn't needed).  Adding "else ;" everywhere seems to just cause bloat.

Still, code style aside, can I suggest the warning be downgraded into a hint? Warnings should indicate the possibility of unstable code due to uninitialised values, for example, and DFA should be able to detect that anyway as a separate warning (e.g. if a case block doesn't initialise an output value in all of its branches).

Gareth aka. Kit

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

It's not DFA.

See my proposal thread from January and Jonas' implementation from this week:

<http://lists.freepascal.org/pipermail/fpc-devel/2019-January/039972.html> <http://lists.freepascal.org/pipermail/fpc-devel/2019-May/040737.html>

Warnings should indicate the possibility of unstable code due to uninitialised values

As long as the core question remains unanswered, these are required exactly because in cases (pun intended) where they exist, the compiler may do something very different from what was intended, and even be wildly different between platforms.

I would prefer literally any other solution (such as the two-line-patch my Windows trunk builds have contained for two years now), but this is the one that gets merged. Go figure.

J. Gareth Moreton wrote

So it looks like this new warning has appeared as part of the data flow analysis of -O4.

The case-completeness warning is not related to any optimization level, nor to the data flow analysis mentioned in the other thread.

The thing is, I personally have a problem with this being a warning, because there's nothing inherently wrong with not covering every case branch or omitting an else block (especially if one isn't needed).  Adding "else ;" everywhere seems to just cause bloat.

The question is whether or not an else block is needed or not (or perhaps even superfluous). When I adapted the compiler code to compile warning-free with this new option, about 2/3 of the cases got an empty else-block and 1/3 got either an internalerror or extra case blocks to handle options that were not handled previously, but that should have been handled. I don't think that's a bad haul. There were also a number of case-statements that handled all possible options and yet still contained an else block (so the else-block got removed).

However, it is absolutely true that this is not an issue in all cases. If you do not wish to see such warnings at all, you can always suppress them using -vm6060 (warning numbers can be discovered with the -vq command line options).

For information on the background of this warning, see http://lists.freepascal.org/pipermail/fpc-devel/2019-January/039972.html

This warning can detect errors that cannot be found by DFA, because they may not be related to initialising a variable (but e.g. to modifying an already initialised variable, or to outputting something).

Jonas

Aah okay.  Thanks Martok and Jonas.  Adding internal errors is definitely a good haul then.  I do think a warning is a bit harsh though over a hint or a note, but if that is what you desire for the compiler (since it does reduce errors), then I'll accept it. I just missed the part where it only does it for small types.

Gareth aka. Kit

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

Ah yes, I see now - I've found the relevant line in ISO7185, section 6.8.3.5:

"One of the case-constants shall be equal to the value of thecase-index upon entry to the case-statement; otherwise, it shall be an error."

Strict, but I guess that was what Pascal was built upon, and a good way to ensure the code is well-behaved.  Besides, I think my label count refactoring that Martok mentioned is quite good at redirecting labels to the endpoint if a branch is empty.  I can't say I fully agree with it being a warning or an error, but I can't argue with an ISO!  I guess I should be grateful that it's only a warning and not a straight-up compiler or run-time error.  I would still ponder about making it a note under FPC and ObjFPC, one level below a warning.

Gareth aka. Kit

Sorry, but you seem to suffer from the "lazy programmer syndrome". A warning is correct, as a case of unhandled */case/* statement can indeed lead to unexpected side effects. While there could be cases where you might want to react with the /*case*/ statement to only certain conditions (and can safely assume that all other conditions don't do ill effects), there is also the case where an unknown/unexpected condition is passed and that can have dire consequences, and should be properly acted upon, by the /*else*/ clause within the case statement...

Ralf

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

Lazy... you've got a bloody cheek.  Well I guess there's no point in me contributing anything more if that's how you honestly view me.  I wasn't aware of the ISO that dictated that a case block shouldn't just fall through until today, and C/C++ and Basic never threw similar errors or warnings, so I didn't know this was a thing.

Good day.

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

Wow, a bit jumpy, aren't we?

As we are talking here about a Pascal compiler, it doesn't matter what C/C++ or BASIC do. And even in other programming languages, properly acting upon unexpected conditions is good programming practice. And commonly people programming in Pascal are ones that try to adhere to good programming practices, not just ignore them out of whatever convenience they might perceive.

A warning from the compiler is hence an appropriate response, telling the programmer to check the source and make a decision, even if it is adding an empty else clause to the offending case statement. Everything else is just "lazy programming", like it or not...

Ralf

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

Then should all if statements have an else? Should constructors give warnings if you don’t initialize all fields? It’s a slippery slope.

Regards,
Ryan Joseph

Yes, I'm jumpy, especially as you always like to open your responses with a direct insult of some kind - yes, I checked.  You want to kill me or ban me from the mailing list, go right ahead. I'm sure Free Pascal will survive without me.

We will agree to disagree on that one.  I see a warning as an actual warning that the code is very likely to cause unpredictable results or is an obvious oversight, like an uninitialised variable or a conditional block that is impossible to branch into (in this case, an 'unreachable else' for the case block would be a warning).  A hint is an optimisation tip that might also indicate laziness, like a variable declaration that's never used because you removed some old code or neglected to use it, while a note is a bit more serious than a hint but not as severe as a warning.

I'm not the only one who wasn't aware of the ISO standard - evidently Borland never followed through with it and most other Pascal programmers.  Until recently, even the compiler was full of omitted blank else blocks.  I would have thought that the lack of an else section on a case block is fairly indictive of one not being necessary, and any special values that need to be handled should contain their own branches.

Now don't get me wrong, I do agree that a notification is useful - as Jonas said, at least 1/3 of the messages pointed to case blocks that benefitted from having internal errors added, since an explicit branch should always be taken - an example of a manual one that Pierre and I implemented is the one that appears in TEntryFile.GetAInt, because SizeOf(AInt) should always return 1, 2, 4 or 8.  But a lot of the time, one may only want to check a select number of values for special operations, but which are numerous to the point that individal if statements are inefficient - for example,
TCpuAsmOptimizer.PeepHoleOptPass1Cpu under x86_64 - having an empty else block, while certainly safer, comes off as superfluous since, in that case, it doesn't give any extra information.  Granted, if the standard of an obligatory 'else' was adhered to from the very beginning, we wouldn't be in this situation, but like the problem of 'inline' appearing only in the implementation section of subroutines, there is so much code out there now that it will be an endless battle to change it all to adhere to the standard and not break something.

I just feel that in this case, it should be a note, not a warning, unless you're compiling under a dialect of Pascal (e.g. Extended) that is stricter in its interpretation of the whitesheet and demands an error be returned.

Gareth aka. Kit

P.S. And while I agree that one should not merely look towards C and C++ for inspiration (I see the practice of lowercasing everything and not having a space between operators and operands as 'colonisation', for lack of a better term), it seems that we are a little inconsistent when it comes to that, given that Free Pascal supports the C-style assignment operators now.

J. Gareth Moreton wrote

Yes, I'm jumpy, especially as you always like to open your responses with a direct insult of some kind - yes, I checked.

Well, I just checked my send folder and this was the first response to you in at least the last two years, if ever...

You want to kill me or ban me from the mailing list, go right ahead. I'm sure Free Pascal will survive without me.

Kill you? You must be smoking some real bad shit. Banning you from the mailing list? Wouldn't be my call in the first place. But certain is that you seem to be taking yourself far to serious.

Yes, an unhandled case value can result in unpredictable results, and that is exactly the reason why there should be a warning from the compiler, not just a "note" or "hint". Just take a language interpreter reading tokens or some data file with data record type identifiers, like spreadsheet cells. If you read a value that isn't defined in your case list, you will and up with unpredictable results.

I'm not the only one who wasn't aware of the ISO standard - evidently Borland never followed through with it and most other Pascal programmers.

For the most part, I don't care about the ISO standard, but what I care about is responsible programming...

Until recently, even the compiler was full of omitted blank else blocks.  I would have thought that the lack of an else section on a case block is fairly indictive of one not being necessary, and any special values that need to be handled should contain their own branches.

Nobody said that there can't be case statements without an else clause. However...

You are contradicting yourself here. If the compiler can determine that there is indeed an unhandled case value, it isn't likely to be superfluous. Yes, there are cases where no else clause is needed in case statement, but then this also should not trigger a compiler warning (or note, hint). If you however get that warning, it is up to the programmer to verify if those situation are possibly harmless, and then it is not an issue to eliminate the warning by adding an empty else clause, at least that indicates in the source that this possibility has been considered. And in cases where unhandled case can not be verified to be harmless, it is prudent to add a more meaningful handling within the else clause.

I don't think that classifying such a message "just as a note" is correct, regardless of the the dialect of Pascal, it is potentially dangerous in any case and hence should be a warning.

Well, the later is certainly not something I petitioned for, despite programming in C almost as long as in Pascal. But what IMHO sets Pascal apart is that it should advocate good programming practices and a Pascal compiler should assist the programmer in writing as clean and error free code as possible. And as such, if the compiler is able to detect a possibly dangerous situation, it should appropriately "warn" the programmer. And then it is still up to the programmer to act upon that warning, or just being lazy and ignore that warning. And re-labeling such a warning as a note or hint is just another form of being lazy. In either case, this doesn't stop the compilation of the code and if either note or warning are ignored, ...

Ralf

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

J. Gareth Moreton schrieb am So., 19. Mai 2019,
05:17:

First of Ralf Quint is not part of the FPC development team, thus he has no power over the mailing list or whether *we* (the development team) accept your contributions.

Second the term "lazy programmer" is a known term in development: https://wiki.c2.com/?LazyProgrammer
So he's not calling you "lazy" personally, but he's pointing out that your argumentation is that of a "lazy programmer".

I just feel that in this case, it should be a note, not a warning,

unless you're compiling under a dialect of Pascal (e.g. Extended) that is stricter in its interpretation of the whitesheet and demands an error be returned.

No, I don't think so. The compiler might generate quite some hints and notes some of which might be false positives (take the "uninitialized variable" in context of "var" parameters) and thus this note/hint would be lost among the noise.

Also you need to keep in mind that enums are not necessarily static over the lifetime of the software. E.g. (using the compiler as an example) a new def type could be added. If you now haven't added empty else-clauses to the case-statements you'll now get a nice warning for every such case statement so that you can be reasonably sure you caught all.

At my work (where we use MSVC) we enabled the compiler's warning for this reason.

P.S. And while I agree that one should not merely look towards C and C++

The C-style operators are merely syntactic sugar and don't do any optimization of the operation. Also they can't be used on properties. Not to mention that not everyone likes their use (I for example am pondering whether I should add a "$COperators Off" to FPC fpcdefs.inc so that contributors won't be tempted to use them).

Regards,
Sven

Op 2019-05-19 om 09:34 schreef Sven Barth via fpc-devel:

Initially they were afaik introduced for conversion purposes. So unless somebody translates LLVM or large parts of GCC, the compiler shouldn't need them :-)

I'm aware of the term "lazy programmer", and to say I suffer from lazy programmer syndrome was still insulting because I pride myself on checking edge cases and making sure my code was thorough and robust (and if I overlook something in particular, I'll gladly have it pointed out so it can be fixed).  Up until now, there was nothing to say that omitting an *else* branch in a *case* block was lazy programming - for all I knew, it made sense because it clearly implied that you wanted nothing special to happen if none of the branches match - but to have the rules changed and the accusation applied retroactively was a grave insult.  Also, I don't drink, smoke or do drugs, and I absolutely abhor swearing, but you don't need to hear my life story.  Still, I apologise for my overreaction.

Back on point, I understand the people have spoken on this being a warning, but I can see this causing a rift among the development community when this warning suddenly gets thrown up on all their projects.  I can see the benefit though, especially with the smaller enumerations.  It just personally seemed that a warning was too harsh, and as Ryan Joseph pointed out, where is the line drawn? When is it acceptable to omit something?  For something like not initialising all of the fields in an object constructor, I will happily have a warning thrown at me because it counts as an uninitialised value, so long as it's not a false positive (since some fields may get set in a different method that's called by the constructor).  However, should not including an *else *branch in an *if* statement throw a warning - one can rightly argue that that's stupid, but isn't the principle the same as the *case *block?

I'm all for Pascal's design in forcing better programming standards, even little things like the assignment and equality operators are designed in such a way to prevent mistakes or subtle backdoors like that infamous one that someone tried to slip into the Linux kernel back in 2003
(https://freedom-to-tinker.com/2013/10/09/the-linux-backdoor-attempt-of-2003/). Also a reason why I am vehemently against adding inline *var* declarations like what was done in the most recent rendition of Delphi, besides it violating the block design of Pascal.

Regarding the C-style assignment operators, some of the packages that come with the compiler use them, and trying to build everything without the -Sc flag will result in errors.  Outside of that, the biggest one that stands out is LazUTF8.pas for Lazarus - for example: Result += (nx
* ONEMASK) >> ((sizeof(PtrInt) - 1) * 8); - that should probably be changed to something a little more Pascal-like, if Inc(Result, (nx * ONEMASK) *shr* ((sizeof(PtrInt) - 1) * 8)); is acceptable.

Gareth aka. Kit

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

J. Gareth Moreton schrieb am So., 19. Mai 2019,
13:18:

The fpcdefs.inc is only used for the compiler.

And don't get me started about ">>". That one is going to annoy me enough once I add support for nested specializations -.-

Regards,
Sven

Sven Barth wrote

And don't get me started about ">>". That one is going to annoy me enough once I add support for nested specializations -.-

Current C++ standards requires a space between the two > signs for nested templates, so I think it would be fine if we were to require that as well. Although I supposed that would be Delphi-incompatible...

Jonas

I think we can argue that adding a space adds to clarity.

Michael.

Op 2019-05-19 om 13:50 schreef Michael Van Canneyt:

I think that is pointless. Most coperator code is objfpc anyway.

Mode delphi should be compatible. Better forbid coperators in combination with mode delphi if need be.

Ralf Quint wrote

A warning from the compiler is hence an appropriate response, telling the programmer to check the source and make a decision, even if it is adding an empty else clause to the offending case statement. Everything else is just "lazy programming", like it or not...

i'm still trying to figure out what "bloat" the OP was talking about :smh:

I was talking about adding "else ;" to every single case block that deliberately didn't have one before.  A lot of them were inserted into the compiler source recently (although a number of the new else branches also trigger internal errors too, since it should never be reached).

For any project that extensively uses case blocks, I felt that adding "else ;" came off as superfluous.  That's why I said it was bloat.

Gareth aka. Kit

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

What makes you think an incomplete case statement might not be a false positive?

A note would do just the same.

That all said, I very much like this new enum warning. A note would just do the same job (and I don't care if it's a warning or a note).

At my work (where we use MSVC) we enabled the compiler's warning for this reason.

In FPC we needed such a construct at every case block

    {$IF (Ord(High(TMyEnum)) 2)} {$MESSAGE Warning 'Case does not respond to all possible values'} {$IFEND}

to get a warning when a new enum value is added. We don't need it anymore, good.

Ondrej

Jonas Maebe schrieb am So., 19. Mai 2019, 13:29:

If with "current" you mean pre-C++11 then you are right. With C++11 they explicitly redefined it so that ending the specialization takes precedence over the operator.

Regards,
Sven

Michael Van Canneyt schrieb am So., 19. Mai 2019, 13:51:

At work when we still used VS 2008 I had to add the space to such declarations which looked simply ugly that way. When we switched to VS 2013 which allowed to drop the space I did just that right away resulting in much more pleasing looking sequences of characters. Please note that this doesn't stop anyone from using a space there, but for me the one without space looks better - especially as the IDE highlights types differently than the brackets.

Regards,
Sven

Marco van de Voort schrieb am So., 19. Mai
2019, 14:04:

I think that we should not forbid the use of COperators if the user want it just like we don't forbid the use of any modeswitch.

What should be done however is to couple the << and >> operator aliases to the COperators option, cause currently they are not and thus are enabled always.

Regards,
Sven

Op 2019-05-19 om 19:24 schreef Sven Barth via fpc-devel:

Possibly. But if it is Delphi compatibility or support Coperators, it should be Delphi compatibility for $mode delphi. That is what the mode is about. Anything else makes dual maintenance of code unnecessarily hard.

Definitely.