logo
  • Buy
  • Download
  • Documentation
  • Blog
  • Contact

Blog

4 Dec 2015

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


  • Francis PARLANT says:
    December 4, 2015 at 1:14 pm

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

    Reply
  • 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

    Reply
  • Rodrigo says:
    December 4, 2015 at 2:15 pm

    Indy ?

    Reply
  • Roman Yankovsky says:
    December 4, 2015 at 5:59 pm

    Hi Olivier,

    I will create a pull request on this weekend.

    Reply

Leave a Reply Cancel reply

Your email address will not be published. Required fields are marked *

  • Announcements
  • DelphiAST
  • DelphiSpec
  • FixInsight
  • FMX
  • Other
  • VCL

Recent Posts

  • Find leaks in Delphi with Deleaker
  • FixInsight and the inline directive
  • FixInsight 2017.04 support Delphi 10.2 Tokyo
  • FixInsight 2016.04 support Delphi 10.1 Berlin
  • FixInsight vs FMX in Delphi 10.1 Berlin

Archives

  • January 2020
  • April 2017
  • April 2016
  • March 2016
  • December 2015
  • November 2015
  • October 2015
  • September 2015
  • August 2015
  • April 2015
  • March 2015
  • February 2015
  • September 2014
  • August 2014
  • January 2014
  • December 2013
  • October 2013

Recent Comments

  • anapa-poseidon3.ru on FixInsight vs RTL
  • Heat pump on FixInsight vs RTL
  • Suing on FixInsight vs RTL
  • JorgeJag on Find leaks in Delphi with Deleaker
  • Prorabdom on FixInsight vs RTL
  • Home
  • Buy
  • Download
  • Documentation
  • Blog
  • Contact
  • © 2014-2015 SourceOddity|
  • Terms and Conditions|
  • Privacy Policy