One of my pet peeves, as you may already know, is leaving memory leaks in “final” code.
Another one is leaving compiler Hints and Warnings.
When doing a fresh build, there should be no Hints and no Warnings. Period. No excuses!
- I know some organizations that ship production code with thousands of Hints and Warnings…
- Some developers even disable Hints and Warnings in the IDE…
- You may think it’s reasonable to deal with Warnings (up to drastic enforcement by turning them into Errors so it won’t even compile when there’s a Warning) and not bother with Hints…
The other day at work, when working on our components migration from pre-Unicode to XE, I had a very example of a seemingly benign Hint that was in fact a bug in waiting… Unlikely to happen because located in an unfrequented code-path (after all, it had never occurred so far) , but the code’s wrong anyway.
In this routine below, used to automatically create a new unique name by adding/incrementing a numeric suffix, the compiler gave a “H2077: Value assigned to ‘n’ never used” Hint on the n := n+1; instruction.
procedure TForm1.SetDefaultName(aGroup: TComponent);
var
n: integer;
nStr: string;
begin
n := GroupList.IndexOf(aGroup);
if n>=0 then begin
n := n+1;
nStr := IntToStr(n);
if Length(nStr)=1 then
nStr := ' '+nStr;
while GroupNameExists('Zone '+nStr) do
n := n+1; // <= [DCC Hint] H2077 Value assigned to 'n' never used
aGroup.Name := 'Zone '+nStr;
end;
end;
At first glance, you’d think the compiler is nuts because n is actually used in this same instruction, but look again…
Got it?
The exit condition in the while instruction is never recalculated using the new value of n, so, either it does not start or it does not stop. Bad!
This needs to be escalated to be at least a prominent Warning in the current code base, and to be sure it cannot be ported “as is” to the new XE version; we want a big fat Error that would prevent the compilation and “break the build” until it’s fixed.
There is a simple way to do it with the $Message directive:
{$Message Hint|Warn|Error|Fatal 'text description'}
and using some conditional define to switch between the Warning or the Error:
while GroupNameExists('Zone '+nStr) do
{$IF (CompilerVersion < 21) or Defined(FG_BYPASS)}
{$Message Warn '** bad loop! **'} {$ELSE} {$Message Error '** bad loop! **'} {$IFEND}
n := n+1; // FG: loop can't stop, nStr does not depend on n!
aGroup.Name := 'Zone '+nStr;
And voila!
The current code still compiles, but now with a Warning, and the new XE version in the works cannot (unless, I override that temporarily with my “magic” define).
Of course, if you want to stop the compilation immediately to highlight the problem, you can use $Message Fatal and compilation will stop right there without any chance to bury the error in some more compiler messages.
Note: the code above is not the actual code, but a simpler example, with the same flaw, for demonstration purposes…
Update / Conclusion
The example and code above should not distract from those 2 main points:
- Even Hints can point to actual bugs and should not be overlooked.
- The $Message directive allows you to fire Warnings from your code or even forbid  using it depending on compiling conditions.
So let me see, you find a bug but instead of fixing it you write more code to tell yourself (or someone else) that there is a bug ?
Why not just fix the damned bug ? 🙂
For the same reason you would put some warning sign if you find a big pothole in the middle of the road to make sure nobody falls into it while the decision to condemn the road, to build a new one or send someone to repair is pending.
I’m still just scouting the code base while everything gets installed and at the same time putting some markers in place. I haven’t yet had a chance to sit with the guy and go over the code base in detail. I’m sure there is a bug, but maybe this code is not even needed, will be removed altogether and this bug will never be fixed per se. Or maybe it’s for a needed (future) feature and the bug will be eventually fixed… One sure thing is that it won’t sneak in and come bite me in the b… later.
There is only one option: remove ANY hint, ANY warning, not doing so is very very stupid. period.
Call me stupid, but if you don’t mind I’ll just turn off the “platform” hints and warnings in my Win32 projects. Ta. 🙂
Very stupid? I don’t think so, consider using the “Val” method to validate input(in TempString) from the user:
val(TempString, TempInt, code);
if (code 0) then exit;
…
I only care that code is non-zero, I don’t even need TempInt.
I get “H2077 Value assigned to ‘TempInt’ never used”. The message is correct, but I don’t care to change it. The fact that there is a hint is insignificant to me, but to build and code Nazis it might not be, so I would rather turn it off with a directive for that line only.
There is another possible bug in the code:
if Length(nStr)=1 then
nStr := ‘ ‘+nStr;
suggests that the newly generated name is formatted to 7 chars length, i.e. n should not exceed 99. No way of telling that without seeing the rest of the code, though.
I agree with Jolyon here. You are already getting what you consider a bug report from the compiler. You’ve just made it far stranger rather than clearer.
If you feel the need to leave a note, might I suggest a more sane simple comment OR, should you feel compelled to revisit the section – use a TODO, which is meant for actual review rather than esoteric custom comments.
In this particular case, since the bug is clearly severe and the context is fairly clear, might I suggest just fixing the freaking thing instead? Then you might want to document the whole silly formatting issue and what happens when you hit 100 items in a TODO.
I would love to ship a hint- and warning-less build but there are still hundreds of them in the third-party code I include (e.g. in the JCL – the majority is of course related to string-conversions)…
@Oliver, you won’t get the JCL hints if you remove the JCL source folders from your library path. Your project will compile quicker too.
@Lachlan: Well, at some point I have to put them on the library path (such as in an automatic build script that pulls everything fresh from source control) – unless I actually checked in the .dcu’s, I guess…
I’d think that if you wished to demonstrate that even Hints can be dangerous, why not select a better code fragment? The above code is just terrible. Apart fom the ‘while’ loop not being related to anything else, it doesn’t handle the case of IndexOf returning -1 and it repeats the name format twice. So the selected code fragment sort of drowns the point you want to make. Why not code it like this
n:=GroupList.IndexOf(aGroup);
OK:=(n<0);
if not OK then begin
repeat
n:=n+1;
sName:='Zone '+Format('%.2d',[n]);
OK:=not GroupNameExists(sName);
until OK;
end
else begin
sName:='Zone 0';
end;
if OK then aGroup.Name:=sName;
Apart from all that, you're probably quite correct in pointing out the possible dangers associated with compiler hints. However, I do feel that having a '0 hint policy' is taking things to far ('o warnings' I do agree with, warnings are nearly always a sign that something is really wrong). I prefer to have code symmetry even if that gives me a 'compiler hint' of unused assignments. I also like to specify what Result will be as the default at the top of a function even if it will be re-assigned further on. Coding preferences that collide with the compiler hints but where I prefer my style and accept the inevitable hints.
Further simplified…
n := Math.Max(0, GroupList.IndexOf(aGroup)); // 0 – n
repeat
Assert(n < 99); // maximum for # of digits in format below
NewName := Format('Zone %.2d', [n]);
Inc(n);
until not GroupNameExists(NewName);
aGroup.Name := NewName;
I prefer to only have assignments where something is changed from its previous value.
As for using warn instead of fixing the bug. If it is a small fix with low risk of side effects – do the fix and flame the other guy. Otherwise add the warn + todo for developer assigned to the fix.
Pingback: Hints and Warnings begone! | A little bit of zis, a little byte of zat