Blog: VCL

FixInsight and the inline directive

The Delphi compiler allows functions and procedures to be tagged with the inline directive to improve performance. If the function or procedure meets certain criteria, the compiler will insert code directly, rather than generating a call. Embarcadero docwiki gives a list of conditions under which inlining does or does not occur.

One of basic conditions says: within a unit, the body for an inline function should be defined before calls to the function are made.

FixInsight 2017.04 introduces rule O805 “Inline marked routine comes after its call in the same unit”. Let’s check FMX and VCL code to see if Embarcadero follows their own rules. Short answer: it doesn’t.

I will give a couple of examples from Delphi 10.1 Berlin (version 24.0.22858.6822).

procedure TWinControl.ReadState(Reader: TReader);
begin
  DisableAlign;
  try
    inherited ReadState(Reader);
  finally
    EnableAlign;
  end;
  FixupTabList;
  if FParent <> nil then Perform(CM_PARENTCTL3DCHANGED, 0, 0);
  UpdateControlState;
end;

procedure TWinControl.AlignControl(AControl: TControl);
var
  Rect: TRect;
begin
  if not HandleAllocated or (csDestroying in ComponentState) then Exit;
  if FAlignLevel <> 0 then
    Include(FControlState, csAlignmentNeeded)
  else
  begin
    DisableAlign;
    try
      Rect := GetClientRect;
      AlignControls(AControl, Rect);
    finally
      Exclude(FControlState, csAlignmentNeeded);
      EnableAlign;
    end;
  end;
end;

procedure TWinControl.DisableAlign;
begin
  Inc(FAlignLevel);
end;

TWinControl.DisableAlign is an inline marked procedure. It is called at line 8531 and line 9019, but its body is defined after the calls – at line 9030. Obviously, this function will not be inlined.

One more example from another unit:

procedure TListViewBase.UpdateDeleteButtonLayout;
var
  RelRect: TRectF;
begin
  if (Adapter.Count < 1) or (FDeleteLayout = nil) or ((FDeleteButtonIndex = -1) and
    (FPrevDeleteButtonIndex = -1)) then
    Exit;

  if (FListingService <> nil) and (TListingTransitionFeature.DeleteButtonSlide in FListingService.GetTransitionFeatures) then
  begin
    FDeleteLayout.Width := DefaultDeleteButtonWidth * FDeleteModeTransitionAlpha;
    FDeleteButton.Opacity := 1;
  end
  else
  begin
    if FDeleteModeTransitionAlpha > 0 then
      FDeleteLayout.Width := DefaultDeleteButtonWidth
    else
      FDeleteLayout.Width := 0;

    FDeleteButton.Opacity := 0.5 + (FDeleteModeTransitionAlpha / 2);
  end;

  FDeleteLayout.Height := GetItemHeight(FDeleteButtonIndex);
  FDeleteLayout.Position.X := Width - FDeleteLayout.Width;

  if FDeleteButtonIndex = -1 then
    RelRect := GetItemRelRect(FPrevDeleteButtonIndex, LocalRect)
  else
    RelRect := GetItemRelRect(FDeleteButtonIndex, LocalRect);

  FDeleteLayout.Position.Y := (RelRect.Top + RelRect.Bottom - FDeleteLayout.Height) / 2;
end;

The method above contains two GetItemRelRect calls (lines 2457 and 2459), but both are before the actual GetItemRelRect body position in that unit (line 2868):

function TListViewBase.GetItemRelRect(const Index: Integer; const LocRect: TRectF;
  const SideSpace: Integer = 0): TRectF;
begin
  Result := RectF(LocRect.Left + FSideSpace + SideSpace, LocRect.Top + FSideSpace + FHeightSums[Index] - FScrollViewPos,
    LocRect.Width - ((SideSpace + FSideSpace) * 2), GetItemHeight(Index));

  if (FScrollBar <> nil) and (not HasTouchTracking) and FScrollBar.Visible then
    Result.Right := Result.Right - FScrollBar.Width;
end;

Despite being declared as inline, this method will not be inlined.

It is not a critical issue, but this makes inline directive useless. There are more occurrences of this issue in Vcl.ExtActns.pas, FMX.ASE.Lexer.pas, FMX.Graphics.pas, FMX.Types.pas, FMX.Utils.pas and FMX.ZOrder.Win.pas.

This means that inlining conditions are not easy to follow, even though at first glance inline directive seems to be an easy way to slightly optimize your code. FixInsight may help to make inlining more useful by avoiding such mistakes.

You can download FixInsight trial and check your own code.

P.S. Thanks to Stefan Glienke for suggesting this new FixInsight rule.

FixInsight vs VCL

Of course I couldn’t resist analyzing Delphi VCL with FixInsight. All examples in this post are related to Delphi XE7 (version 21.0.17017.3725). I will try to choose the most interesting places and ignore minor issues like methods that are too long or nested “with” statements (have you ever thought how many nested “with” statement in VCL?).

W512 Odd ELSE-IF condition

    if Element = sbGroupStartInactive then
      LSrcRect := System.Types.Rect(0, 110, 22 - 2, 132)
    else if Element = sbGroupEndInactive then
      LSrcRect := System.Types.Rect(22, 110, 46 - 2, 132)
    else if Element = sbGroupEndInactive then
      LSrcRect := System.Types.Rect(47, 110, 70 - 2, 132)
    else
      LSrcRect := System.Types.Rect(71, 110, 95 - 2, 132);
    DrawButton(RibbonSkin.Bitmap, LSrcRect, Canvas, Rect, 2, 2, 255);

Pay attention to lines 809 and 811. These conditions are equal. There is obviously something wrong. How could we fix this?

Let’s look around:

  else if Element in [sbGroupStartInactive, sbGroupMiddleInactive,
    sbGroupEndInactive, sbGroupSingleInactive] then
  begin
    if Element = sbGroupStartInactive then
      LSrcRect := System.Types.Rect(0, 110, 22, 132)
    else if Element = sbGroupMiddleInactive then
      LSrcRect := System.Types.Rect(22, 110, 46, 132)
    else if Element = sbGroupEndInactive then
      LSrcRect := System.Types.Rect(47, 110, 70, 132)
    else
      LSrcRect := System.Types.Rect(71, 110, 95, 132);
    DrawButton(RibbonSkin.Bitmap, LSrcRect, Canvas, Rect, 2, 2, 255);
  end
  else if Element in [sbGroupStartSplitInactive, sbGroupMiddleSplitInactive,
    sbGroupEndSplitInactive, sbGroupSingleSplitInactive] then
  begin
    if Element = sbGroupStartInactive then
      LSrcRect := System.Types.Rect(0, 110, 22 - 2, 132)
    else if Element = sbGroupEndInactive then
      LSrcRect := System.Types.Rect(22, 110, 46 - 2, 132)
    else if Element = sbGroupEndInactive then
      LSrcRect := System.Types.Rect(47, 110, 70 - 2, 132)
    else
      LSrcRect := System.Types.Rect(71, 110, 95 - 2, 132);
    DrawButton(RibbonSkin.Bitmap, LSrcRect, Canvas, Rect, 2, 2, 255);
  end
  else if Element in [sbSmallSplitHoverSplit, sbSmallSplitPressedSplit,
    sbSmallSplitInactiveSplit] then

That’s even more interesting. Along with “Element in [sbGroupStartSplitInactive, sbGroupMiddleSplitInactive, sbGroupEndSplitInactive, sbGroupSingleSplitInactive]” condition (see line 804), that code looks even more strange. I guess it is a rought copy-paste of the code above (lines 794 – 802) and actually it is intended to look like this:

  else if Element in [sbGroupStartSplitInactive, sbGroupMiddleSplitInactive,
    sbGroupEndSplitInactive, sbGroupSingleSplitInactive] then
  begin
    if Element = sbGroupStartSplitInactive then
      LSrcRect := System.Types.Rect(0, 110, 22 - 2, 132)
    else if Element = sbGroupMiddleSplitInactive then
      LSrcRect := System.Types.Rect(22, 110, 46 - 2, 132)
    else if Element = sbGroupEndSplitInactive then
      LSrcRect := System.Types.Rect(47, 110, 70 - 2, 132)
    else
      LSrcRect := System.Types.Rect(71, 110, 95 - 2, 132);
    DrawButton(RibbonSkin.Bitmap, LSrcRect, Canvas, Rect, 2, 2, 255);
  end

I don’t have much experience with Ribbon controls, so the fix offered solely on the basis of common sense. Maybe someone can do it better.

OK, go ahead.

W507 THEN statement is equal to ELSE statement

It’s Ribbon again :)

procedure TRibbonDropDownButton.DrawBackground(var PaintRect: TRect);
var
  LPt: TPoint;
begin
  inherited;
  LPt := GetArrowPosition(PaintRect);
  if not Enabled then
    DrawArrow(LPt, clNone, clNone, -1)
  else
    DrawArrow(LPt, clNone, clNone, -1);
end;

No comments. Obviously, the code in “then” and “else” are the same. It’s not clear what was meant to be there.

A similar issue in another file:

      if Assigned(Clients[I].ActionBar) then
      begin
        if Assigned(ActionProc) then
          ActionProc(Clients[I])
        else
          Clients[I].Refresh;
        UpdateActionBar(Clients[I]);
      end
      else
      begin
        if Assigned(ActionProc) then
          ActionProc(Clients[I])
        else
          Clients[I].Refresh;
        UpdateActionBar(Clients[I]);
      end;

And one more:

        if Style <> csSimple then
          if DroppedDown then
            DrawFrameControl(C.Handle, R, DFC_SCROLL, DFCS_FLAT or DFCS_SCROLLCOMBOBOX)
          else
            DrawFrameControl(C.Handle, R, DFC_SCROLL, DFCS_FLAT or DFCS_SCROLLCOMBOBOX);

OK, next.

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

function CheckPoint(const Point, Source: TPoint; Deviation,
  ErrorMargin: Integer): Double;
var
  Distance: Double;
  ErrorMarginDistance: Double;
  M, B: Double;
begin
  Deviation := Deviation;
  Distance := Sqrt(Sqr(Point.X - Source.X) + Sqr(Point.Y - Source.Y));
  ErrorMarginDistance := MulDiv(Deviation, ErrorMargin, 100);

This assignment either should be different or it is completely useless.

And one more:

procedure TScreenTipsWindow.CreateParams(var Params: TCreateParams);
begin
  inherited CreateParams(Params);
  with Params do
  begin
    Style := WS_POPUP;
    WindowClass.Style := WindowClass.Style;
    if NewStyleControls then
      ExStyle := WS_EX_TOOLWINDOW;
    if CheckWin32Version(5, 1) then
      WindowClass.Style := WindowClass.Style or CS_DROPSHADOW;
    if NewStyleControls then
      ExStyle := WS_EX_TOOLWINDOW;
    AddBiDiModeExStyle(ExStyle);
  end;
end;

And last, but not least…

W504 Missing INHERITED call in destructor

There are too many messages of this type, so I’m not going to quote everything.

destructor TActiveXPropertyPage.Destroy;
begin
  FPropertyPageImpl.FPropertyPage.Free;
  FPropertyPageImpl.Free;
end;

There is no “inherited” call, ancestor’s destructor is never called.

Well, let’s finish. As I said at the beginning, I don’t want to nitpick, so I ignored minor issues. In the next episode, we may talk about FMX or RTL :)

Static analysis is cool, isn’t it?