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? :)