FixInsight vs JEDI

by Roman Yankovsky 4 Comments

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

JclMsBuild.pas// 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));

JclMsBuild.pas
1493 1494 1495 1496 1497 1498 1499 1500 1501 1502 1503 1504 1505 1506 1507 1508 1509 1510 1511 1512 1513 1514 1515 1516 1517 1518	`// 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

JclGraphics.pasprocedure 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;

JclGraphics.pas
2185 2186 2187 2188 2189 2190 2191 2192 2193 2194 2195 2196 2197 2198 2199	`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

JvgMultiResources.pasfor J := 0 to Comps.Count - 1 do
  if Pos(NameOfC + '.', Comps[I]) = 1 then
    Comps.Objects[I] := C;

JvgMultiResources.pas
148 149 150	`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

JvgLogicsEditorForm.pastry
  glLogicsEditor := TJvgLogicsEditorMain.Create(nil);
  glLogicsEditor.Execute(LogicProducer);
finally
  FreeAndNil(glLogicsEditor);
end;

JvgLogicsEditorForm.pas
692 693 694 695 696 697	`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

JvAVICapture.pas    // 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;

JvAVICapture.pas
925 926 927 928 929 930 931 932 933 934 935 936 937 938 939 940 941 942 943 944 945 946 947 948 949	`// 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? :)

Francis PARLANT says:

December 4, 2015 at 1:14 pm

FixInsight seems to be a good candidate for the next analysis no ?… :-)

OBones says:

December 4, 2015 at 1:26 pm

Hello,

Thanks for this analysis, when I tried it I couldn’t get the free version to work with the number of source files we have.
What would be really nice is if someone could create pull requests for these.

Regards,
Olivier

Rodrigo says:

December 4, 2015 at 2:15 pm

Indy ?

Roman Yankovsky says:

December 4, 2015 at 5:59 pm

Hi Olivier,

I will create a pull request on this weekend.

Blog

FixInsight vs JEDI

Leave a Reply to Roman Yankovsky Cancel reply