Another reason why hints can be a sign of smelly code
Another reason why hints can be a sign of smelly code
Daniela Osterhagen pointed out that hints like "var declared but not used" could be a hint of a reference mistake, and today I stumbled on another reason why hints should not be ignored.
[dcc32 Hint] somefilename.pas(666): H2077 Value assigned to 'r' never used
begin
ZeroMemory(@w32fd, Sizeof(TWin32FindData));
w32fd.dwFileAttributes := FILE_ATTRIBUTE_DIRECTORY;
ifs := TFileSystemBindData.Create;
ifs.SetFindData(w32fd);
r := CreateBindCtx(0, pbc);
r := pbc.RegisterObjectParam(STR_FILE_SYS_BIND_DATA, ifs);
end;
In this case, the hint reveals that error handling is missing for these two calls. The actual calls in this specific snippet might not be error prone, but ...
The point is: A "value never used" hint may indicate potential flow control flaws, resulting in stability or security pitfalls.
Daniela Osterhagen pointed out that hints like "var declared but not used" could be a hint of a reference mistake, and today I stumbled on another reason why hints should not be ignored.
[dcc32 Hint] somefilename.pas(666): H2077 Value assigned to 'r' never used
begin
ZeroMemory(@w32fd, Sizeof(TWin32FindData));
w32fd.dwFileAttributes := FILE_ATTRIBUTE_DIRECTORY;
ifs := TFileSystemBindData.Create;
ifs.SetFindData(w32fd);
r := CreateBindCtx(0, pbc);
r := pbc.RegisterObjectParam(STR_FILE_SYS_BIND_DATA, ifs);
end;
In this case, the hint reveals that error handling is missing for these two calls. The actual calls in this specific snippet might not be error prone, but ...
The point is: A "value never used" hint may indicate potential flow control flaws, resulting in stability or security pitfalls.
A simple rule I am following for many years now is "Code with hints or warnings is bad code" (not saying that code without them is good ^^).
ReplyDeleteI wish the compiler emitted /more/ hints - there are sometimes things I think it should warn about that it doesn't.
ReplyDeleteIf we quantify which additional hints we'd like, we can always suggest them to Marco and team.
ReplyDeleteDid you have any specific hint in mind, David Millington ?
Lars Fosdal The quirk where functions which return managed types (strings, interfaces) don't cause the 'result may be undefined' warning springs to mind. I noticed it sometimes happens for booleans, too - no idea why. (XE6.)
ReplyDeleteAlso, to pick something very random: if a class has a single constructor defined but it is protected, and you then construct an instance of that class from another unit, you call the class's ancestor's constructor only. Makes sense because the protected constructor is inaccessible, but it happens silently. If a constructor exists, it's probably meant to be used, so the lack of a public constructor is a possible symptom of a bug. (It's probably not what you intended: you either put the constructor in the protected section by mistake, or you were following C++ protection rules and thought it made instances of the class constructable only from within that unit.)
So, even though it's an odd situation, a warning would be more appropriate than silently compiling code that leaves an object in half-constructed state.
There are others... I run across things every so often. I should keep a list!
Oh, that's some nasty ones.
ReplyDeleteIf they are not already in the QC, they should be. What say Marco Cantù ?