Hi all,
Someone reported bug in gtk2lcl which crashes lazarus ide. I've found that
intersectRect usage from types (from r 27870) makes that.
So imagine next code:
var
  R, SrcRect: TRect;
begin
   R := Rect(0, 0, 22, 22);
   SrcRect := Rect(0, 0, 24, 24);
   Types.IntersectRect(R, SrcRect, R);
end;
In this case RETURNED value from intersectRect will be wrong R = 0, 0, 24, 24.
Now the question is: first param of Intersect is var, others are consts.
function IntersectRect(var Rect : TRect;const R1,R2 : TRect) : Boolean;
begin
  Rect:=R1; // <--------- WHAT IF Rect = R2 as in my example above ?
  with R2 do
    begin
    if Left>R1.Left then
      Rect.Left:=Left;
    if Top>R1.Top then
      Rect.Top:=Top;
    if Right<R1.Right then
      Rect.Right:=Right;
    if Bottom<R1.Bottom then
      Rect.Bottom:=Bottom;
    end;
  if IsRectEmpty(Rect) then
    begin
    FillChar(Rect,SizeOf(Rect),0);
    IntersectRect:=false;
    end
  else
    IntersectRect:=true;
end;

So MAIN question is:
1.My example shows misusage of IntersectRect
2.FPC Types.IntersectRect() have bug

thanks,
zeljko

IMHO it is

2.FPC Types.IntersectRect() have bug

Felipe Monteiro de Carvalho wrote

IMHO it is > 2.FPC Types.IntersectRect() have bug

And it's very ugly :)
I've grepped complete lazarus tree for such "misusage" of IntersectRect, but found only same problematic code in gtk dir and fixed that.

zeljko

zeljko wrote

No, I've opened an issue and Jonas closed with good explanation - we had misusage of IntersectRect in gtk/gtk2 interfaces (I've fixed both) ... so not bug.

I guess it depends on how you see it.

Anyway, the solution would be trivial, just store the inputs in local vars.

Felipe Monteiro de Carvalho wrote

IMHO it is > 2.FPC Types.IntersectRect() have bug

No, I've opened an issue and Jonas closed with good explanation - we had misusage of IntersectRect in gtk/gtk2 interfaces (I've fixed both) ... so not bug.

zeljko

I know, but generally speaking Jonas is correct, but you too :). I don't like current fpc solution too, but can live with it.

zeljko

zeljko пишет:

With a little testing, here are two facts:
1) Delphi 7 also shows this bug.
2) Windows.IntersectRect() does not show it, allowing 'misuses'.

So, whoever wrote the FPC function, must have been trying really hard to achieve bug-to-bug compatibility with Delphi... or simply copied their buggy code.

Regards,
Sergei

Not necessarily. It's probably a consequence of the person not seeing a reason for first doing everything in a temporary variable and then copying this temporary variable to the destination. Unless you consciously think about the fact that const parameters may be passed by reference, it's easy to make such "mistakes" (although technically, they are not mistakes, but they do make it easier for people using such a function to make error).

Jonas

Jonas Maebe пишет:

Well, maybe. For me, it is rather hard to think about the opposite - that a const may be passed *not* by reference - due to long-term Delphi/Windows experience.

But, after all, Types.IntersectRect and surrounding stuff were introduced and are intended as WinApi replacement, and WinApi IntersectRect() *does* pass params by reference (they are originally PRect). So maybe it will be still good to fix Types.IntersectRect to conform?

Regards,
Sergei

Yes. It should be improved.

Michael.

One note: IntersectRect in lazarus worked OK until we decided to use Types.IntersectRect, old implementation from GraphType looks much better than current fpc one.

zeljko

Then it is probably better to undo the change.
Should I do that?

Mattias

I think yes, but should be removed when Lazarus uses the fpc with the new IntersectRect version, since even reverting now it will still have the potential to lead to obscure bugs.
In the same program you can have IntersectRect with different behavior just if you switch the uses order LCLIntf Types

PS: I was thinking of removing the other related functions (InflateRect) but gave up since they are also different. I will wait for the next Lazarus release and see first if Types version is Delphi/LCL compatible.

Luiz

Quoting Mattias Gaertner :

No, why ? I've fixed problems inside widgetsets.Let's propose patch for improving Types.IntersectRect().

zeljko

Quoting Luiz Americo Pereira Camara :

This is mainly why I don't won't undo IntersectRect() to laz one.Better to propose good patch for Types.IntersectRect().

No, why ? I've fixed problems inside widgetsets.Let's propose patch for improving Types.IntersectRect().

I already fixed this in FPC 2.5.1+ Please verify.

Since the fixed Types.IntersectRect won't ship with fpc242 anyway the potential for problems will exist.

In the case the LCL version is kept removed, an user code misusing IntersectRect may break since will start to uses the Types version.

In the case the LCL version is restored there's the already cited problem of mixing both.

There's two options IMO if LCL version is removed:
- Fix Types.IntersectRect in 242
- Document the misuse behavior so the user can be warned (my preferred).

Luiz