Blog

FixInsight vs FMX

In this post I will continue to analyze Delphi source code. Previous episodes was about Delphi VCL and RTL. As usual I will ignore minor issues and will try to find out the most interesting pieces of code. All code examples in this post are related to Delphi XE8 (version 22.0.19027.8951).

W517 Variable hides a class field, method or property

procedure TLight.ReadDiffuse(Reader: TReader);
var
  Color: Integer;
begin
  IdentToAlphaColor(Reader.ReadIdent, Color);
  {$R-}
  Color := TAlphaColor(Color);
  {$R+}
end;

The same issue in other unit.

procedure TStrokeCube.ReadDiffuse(Reader: TReader);
var
  Color: Integer;
begin
  IdentToAlphaColor(Reader.ReadIdent, Color);
  {$R-}
  Color := TAlphaColor(Color);
  {$R+}
end;

That’s one of those “variable hides a class field” warnings I mentioned in previous post. This case is definetely a bug. TLight class, as well as TStrokeCube class, has Color property (it’s TAlphaColor, of course). So I guess it should be “Self.Color := TAlphaColor(Color)”, but better rename the variable. It’s a very bad style to use the same names for properties and variables. Usually it doesn’t harm, but sometimes leads to hard to find bugs. Just look at this code and don’t do that, ever.

W511 Object created in TRY block

begin
  if ([csDesigning, csDestroying, csLoading, csUpdating] * ComponentState <> []) or
     (FUpdating > 0) then Exit;
  { Update objects in form }
  try
    Comparer := TComparerTFmxObject.Create;
    ClientList := TTObjInfoList.Create(Comparer);
    Bucket := TDictionary<TObject, TObject>.Create;
    for InitiatedCount := 0 to 7 do
    begin
      if CollectActionClients(ClientList) = 0 then
        Break;
      ClientList.Sort;
      for I := 0 to ClientList.Count - 1 do
        ClientList[I].ActionClient.InitiateAction;
      ClientList.Clear;
    end;
  finally
    FreeAndNil(ClientList);
    FreeAndNil(Bucket);
  end;
end;

Application may raise an exception before an object instance is actually assigned to ClientList and/or Bucket variables. That means the finally block below will try to free the memory which is unassigned.

W523 Interface declared without a GUID

  IModelImporter = interface
    function GetDescription: string;
    function GetExt: string;

    function LoadFromFile(const AFileName: string; out AMesh: TMeshDynArray;
      const AOwner: TComponent): Boolean;
  end;

  IFMXUISwitch = interface(UISwitch)
    { Touches }
    procedure touchesBegan(touches: NSSet; withEvent: UIEvent); cdecl;
    procedure touchesCancelled(touches: NSSet; withEvent: UIEvent); cdecl;
    procedure touchesEnded(touches: NSSet; withEvent: UIEvent); cdecl;
    procedure touchesMoved(touches: NSSet; withEvent: UIEvent); cdecl;
    procedure ValueChanged; cdecl;
  end;

Not a bug, actually. But since the interface does not have a GUID, it cannot be used with Supports function or with As operator. Maybe it worth adding a GUID, why not?

W510 Values on both sides of the operator are equal

function SamePosition(const APosition1, APosition2: TPosition): Boolean; overload;
begin
  Result := (APosition1.X = APosition2.X) and (APosition1.Y = APosition1.Y);
end;

I guess, it should be “APosition1.Y = APosition2.Y”.

W508 Variable is assigned twice successively

  FPixelShader := TShaderManager.RegisterShaderFromData('gouraud.fps', TContextShaderKind.PixelShader, '', [
    TContextShaderSource.Create(TContextShaderArch.DX9, [
      $00, $02, $FF, $FF, $FE, $FF, $32, $00, $43, $54, $41, $42, $1C, $00, $00, $00, $9F, $00, $00, $00, $00, $02, $FF, $FF, $03, $00, $00, $00, $1C, $00, $00, $00, $00, $01, $00, $20, $98, $00, $00, $00,
// skipped

  FPixelShader := TShaderManager.RegisterShaderFromData('gouraud.fps', TContextShaderKind.PixelShader, '', [
    TContextShaderSource.Create(TContextShaderArch.DX9, [
      $00, $02, $FF, $FF, $FE, $FF, $32, $00, $43, $54, $41, $42, $1C, $00, $00, $00, $9F, $00, $00, $00, $00, $02, $FF, $FF, $03, $00, $00, $00, $1C, $00, $00, $00, $00, $01, $00, $20, $98, $00, $00, $00,
// skipped

Two assignments in a row, probably not an issue, just a sloppy copy-paste.

W503 Assignment right hand side is equal to its left hand side

  if X1 > X2 then
    X1 := X1;
  if Y1 > Y2 then
    Y1 := Y2;

I guess, it should be “X1 := X2”.

      if (Self.Form <> nil) and (Self.Form.Handle <> nil) then
        Self.Form := Self.Form;

Not sure what was meant to be there (Self.Form is a record field).

And one more.

      if FNew.FFrequency <> 0 then
        FNew.FValue := Round(FNew.FValue / FNew.FFrequency) * FNew.FFrequency
      else
        FNew.FValue := FNew.FValue;

W510 Values on both sides of the operator are equal

            if RegionSize = RegionSize then
            begin
              SetLength(UpdateRects, RegionData.rdh.nCount);
              for i := 0 to RegionData.rdh.nCount - 1 do
              begin
                R := PRgnRects(@RegionData.buffer[0])[i];
                UpdateRects[i] := RectF(R.Left, R.Top, R.Right, R.Bottom);
              end;
            end;

Not sure what was meant to be there.

W513 Format parameter count mismatch

function TCustomValueRange.GetNamePath: string;
begin
  Result := Format( 'Value: %0:*.*f (%1:*.*f..%2:*.*f)', [Value, Min, Max]);
end;

The format string is incorrect. This code will raise an exception.

W505 Empty THEN block

      if FoundValue.Count > 1 then
      else if FoundValue.Count > 0 then
        PropValues[Name] := FoundValue[0];

This looks strange, perhaps it could be replaces with a simple “If FoundValue.Count = 1 then PropValues[Name] := FoundValue[0]”.

Well, that’s all I have to bring today. Use FixInsight to find bugs in your code before your customers do :)