December | 2015 | SourceOddity

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

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

FixInsight vs JEDI

FixInsight & Parnassus Navigator