FixInsight vs JEDI
In this post I will analyze famous JEDI JCL and JVCL source code. I took the latest master branch (December 4, 2015) from GitHub. Previous episodes was about Delphi FMX, VCL and RTL. Now let’s give Embarcadero a break and take a look at some 3d-party code. As usual I will ignore minor issues and focus on the most interesting parts of code.
W508 Variable is assigned twice successively
// MSBuildNodeCount
Properties.ReservedProperties.Values['MSBuildNodeCount'] := '1';
// MSBuildLastTaskResult
Properties.ReservedProperties.Values['MSBuildLastTaskResult'] := 'true';
// MSBuildOverrideTasksPath
// supported only in .net 4.0
// MSBuildProgramFiles32
Path := GetSpecialFolderLocation(CSIDL_PROGRAM_FILESX86);
if Path = '' then
Path := GetSpecialFolderLocation(CSIDL_PROGRAM_FILES);
Properties.ReservedProperties.Values['MSBuildProgramFiles32'] := Path;
// MSBuildProjectDirectoryNoRoot
Path := PathRemoveSeparator(ExtractFilePath(ProjectFileName));
if PathIsAbsolute(Path) and not PathIsUNC(Path) and (Path <> '') and CharIsDriveLetter(Path[1]) then
Path := Copy(Path, 3, Length(Path) - 2);
Properties.ReservedProperties.Values['MSBuildProjectDirectoryNoRoot'] := Path;
// MSBuildThisFile
Properties.ReservedProperties.Values['MSBuildProjectDirectoryNoRoot'] := CurrentFileName;
// MSBuildThisFileDirectory
Properties.ReservedProperties.Values['MSBuildProjectDirectoryNoRoot'] := PathRemoveSeparator(ExtractFilePath(CurrentFileName));
It seems to be a copy-and-paste issue.
I think last two assignments should be replaced with this:
// MSBuildThisFile Properties.ReservedProperties.Values['MSBuildThisFile'] := CurrentFileName; // MSBuildThisFileDirectory Properties.ReservedProperties.Values['MSBuildThisFileDirectory'] := PathRemoveSeparator(ExtractFilePath(CurrentFileName));
W507 THEN statement is equal to ELSE statement
procedure ScreenShot(bm: TBitmap; FormToPrint: TCustomForm; ControlToPrint: TWinControl); overload;
begin
if FormToPrint <> nil then
begin
if (ControlToPrint is TWinControl) then
ScreenShot(bm, FormToPrint, ControlToPrint.Name)
else
raise EJclGraphicsError.CreateResFmt(@RSInvalidControlType,[ControlToPrint.Name])
end
else
if ControlToPrint <> nil then
raise EJclGraphicsError.CreateResFmt(@RSInvalidFormOrComponent, ['form'])
else
raise EJclGraphicsError.CreateResFmt(@RSInvalidFormOrComponent, ['form'])
end;
What’s the purpose of this if-condition? I’m not sure, but I think it can be replaced with this:
if ControlToPrint <> nil then
ScreenShot(bm, ControlToPrint)
else
raise EJclGraphicsError.CreateResFmt(@RSInvalidFormOrComponent, ['form'])
Also take a look at line 2189. Is this check needed? ControlToPrint parameter is already declared as TWinControl.
W528 Variable ‘J’ not used in FOR-loop
for J := 0 to Comps.Count - 1 do
if Pos(NameOfC + '.', Comps[I]) = 1 then
Comps.Objects[I] := C;
A typo. Variable ‘J’ should be used inside this loop, not ‘I’.
W511 Object ‘glLogicsEditor’ created in TRY block
try
glLogicsEditor := TJvgLogicsEditorMain.Create(nil);
glLogicsEditor.Execute(LogicProducer);
finally
FreeAndNil(glLogicsEditor);
end;
No, it’s not that case when you set a variable to nil before a try block :)
W503 Assignment right hand side is equal to its left hand side
// udapte internal settings
with Parms do
begin
FFrameDelay := dwRequestMicroSecPerFrame;
// FFramesPerSec := 1/dwRequestMicroSecPerFrame*1E6;
FConfirmCapture := fMakeUserHitOKToCapture;
FPercentDropForError := wPercentDropForError;
FYield := FYield;
FNumVideoBuffer := wNumVideoRequested;
FCaptureAudio := FCaptureAudio;
FNumAudioBuffer := wNumAudioRequested;
FAbortLeftMouse := FAbortLeftMouse;
FAbortRightMouse := FAbortRightMouse;
FKeyAbort := vKeyAbort;
FLimitEnabled := FLimitEnabled;
FTimeLimit := wTimeLimit;
FStepCapture2x := fStepCaptureAt2x;
FStepCaptureAverageFrames := wStepCaptureAverageFrames;
FAudioBufferSize := dwAudioBufferSize;
FAudioMaster := (AVStreamMaster = AVSTREAMMASTER_AUDIO);
FMCIControl := FMCIControl;
FMCIStep := fStepMCIDevice;
FMCIStartTime := dwMCIStartTime;
FMCIStopTime := dwMCIStopTime;
end;
That’s why I’m always trying to avoid with-statements. FYield, FCaptureAudio, FAbortLeftMouse, FAbortRightMouse, FLimitEnabled and FMCIControl are class fields that are being shadowed by Parms record fields. As a result, these assignments do nothing. To make this work correctly, this code should be rewritten without ‘with’ in this way:
FFrameDelay := Parms.dwRequestMicroSecPerFrame;
FConfirmCapture := Parms.fMakeUserHitOKToCapture;
FPercentDropForError := Parms.wPercentDropForError;
FYield := Parms.FYield;
// etc
That’s all so far.
Even popular open source libraries used by thousands day by day have bugs that can be found with static analysis. A good reason to use FixInsight.
By the way, only in this December, you can save $40 by getting a bundle of FixInsight and Parnassus Navigator.
What library/framework should be the next one? :)