Funny Code in System.Types

[Fritzew 51]

Found today in System.Types........

 

class operator TPoint.Explicit(Value: TPoint): TSmallPoint;
begin
  if Value.x < Low(SmallInt) then
    Result.x := Low(SmallInt)
  else if Value.x > High(SmallInt) then
    Result.x := High(SmallInt)
  else
    Result.x := SmallInt(Result.x);

  if Value.y < Low(SmallInt) then
    Result.y := Low(SmallInt)
  else if Value.y > High(SmallInt) then
    Result.y := High(SmallInt)
  else
    Result.y := SmallInt(Result.y);
end;

Useless and wrong Code...

 

[Sherlock 663]

Would you prefer an exception?

[Mahdi Safsafi 225]

Wow:

Result.x := SmallInt(Result.x);

 

[David Heffernan 2345]

4 minutes ago, Mahdi Safsafi said:

Wow:

Result.x := SmallInt(Result.x);

 

Obviously that cast is a bit pointless but it hardly matters 

[Attila Kovacs 629]

Should be SmallInt(Value.<x/y>);

[Arnaud Bouchez 407]

Type-casting to Smallnt() won't be the same as the origin code, for very small or very big values (e.g. x>32768).

 

Of course, no screen has a 32k horizontal resolution yet... but it may sooner than expected...

[PeterBelow 238]

49 minutes ago, David Heffernan said:

Obviously that cast is a bit pointless but it hardly matters 

The point is that the code is plain wrong, the last else in both if trees needs to cast value.x and value.y, respectively, not result.x/y, which has not been set at those points!

[WillH 33]

Are Emba doing any unit testing? Clearly, either this isn't tested or isn't sufficiently tested.

Is any of the RTL being unit tested?

[Mahdi Safsafi 225]

1 hour ago, David Heffernan said:

Obviously that cast is a bit pointless but it hardly matters 

Not just the cast ...  Result.X := Result.X !

[Guest]

46 minutes ago, WillH said:

Are Emba doing any unit testing?

Yes, they call it "customers" - or was it "cash cows"? 

Edited July 8, 2019 by Guest

[David Heffernan 2345]

1 hour ago, Mahdi Safsafi said:

Not just the cast ...  Result.X := Result.X !

Er, yes, I see it now!! 

[Der schöne Günther 316]

2 hours ago, WillH said:

Are Emba doing any unit testing? Clearly, either this isn't tested or isn't sufficiently tested.

Is any of the RTL being unit tested?

I think it was Nick Hodges who called out for Embarcadero to make their unit tests open source and let the community contribute. Unfortunately, not much happened.

 

Personally, I assume that there are unit tests, but only very few. Otherwise, things like that System.Generics.Collections disaster with XE8 certainly wouldn't have happened...

Edited July 8, 2019 by Der schöne Günther

[Guest]

5 hours ago, Fritzew said:

class operator

I do not use these. But if they do not set Result implicitly then this

Quote

Result.x := SmallInt(Result.x)

is horrendous. If that is a word. Horrific? Wow.

[Georgge Bakh 29]

5 hours ago, WillH said:

Are Emba doing any unit testing? Clearly, either this isn't tested or isn't sufficiently tested.

Is any of the RTL being unit tested?

A bit of static analysis should be enough in that case.

[Stefan Glienke 2002]

It's also a bit irritating that Delphi with all its different sized ordinal types does not have built-in clamp functions to this day...

[David Heffernan 2345]

1 hour ago, Stefan Glienke said:

It's also a bit irritating that Delphi with all its different sized ordinal types does not have built-in clamp functions to this day...

EnsureRange in the RTL, Math unit IIRC

Edited July 8, 2019 by David Heffernan

[Arnaud Bouchez 407]

oh I missed the result.x := result.x assignment! 😄

[Stefan Glienke 2002]

12 hours ago, David Heffernan said:

EnsureRange in the RTL, Math unit IIRC

You are right - now I wonder why that was not used  - probably the author of that code also forgot about those 😉

[Dalija Prasnikar 1396]

19 minutes ago, Stefan Glienke said:

You are right - now I wonder why that was not used  - probably the author of that code also forgot about those 😉

I keep rediscovering those every time I go and write my own... done that few times already...

[eivindbakkestuen 47]

On 7/8/2019 at 9:30 PM, Fritzew said:

Found today in System.Types........

 

class operator TPoint.Explicit(Value: TPoint): TSmallPoint;
begin
  if Value.x < Low(SmallInt) then
    Result.x := Low(SmallInt)
  else if Value.x > High(SmallInt) then
    Result.x := High(SmallInt)
  else
    Result.x := SmallInt(Result.x);

  if Value.y < Low(SmallInt) then
    Result.y := Low(SmallInt)
  else if Value.y > High(SmallInt) then
    Result.y := High(SmallInt)
  else
    Result.y := SmallInt(Result.y);
end;

Useless and wrong Code...

 

Post the Quality Portal link, so we can vote and perhaps it'll be fixed...

[Remy Lebeau 1393]

3 hours ago, eivindbakkestuen said:

Post the Quality Portal link, so we can vote and perhaps it'll be fixed...

https://quality.embarcadero.com/browse/RSP-24834

 

[Fritzew 51]

20 minutes ago, Remy Lebeau said:

https://quality.embarcadero.com/browse/RSP-24834

 

Thanks Remy, not only for this also of your work over all the years now 

[stijnsanders 35]

Guys, are all of you missing this? Due to the Pascal calling convention, the first (plain!) argument of a function maps into the same register(s), so in fact this is valid and correct code. Though strictly I agree it looks weird and like as if in 'normal' cases the Value members aren't assigned to Result members. Bit in fact, they're already there! So what is actually needed is a 'type size limiting' cast, which is exactly what Result.x:=SmallInt(Result.x); is.

[David Heffernan 2345]

12 hours ago, stijnsanders said:

Guys, are all of you missing this? Due to the Pascal calling convention, the first (plain!) argument of a function maps into the same register(s), so in fact this is valid and correct code. Though strictly I agree it looks weird and like as if in 'normal' cases the Value members aren't assigned to Result members. Bit in fact, they're already there! So what is actually needed is a 'type size limiting' cast, which is exactly what Result.x:=SmallInt(Result.x); is.

Win64 anyone? 

[Fritzew 51]

12 hours ago, stijnsanders said:

Guys, are all of you missing this? Due to the Pascal calling convention, the first (plain!) argument of a function maps into the same register(s), so in fact this is valid and correct code. Though strictly I agree it looks weird and like as if in 'normal' cases the Value members aren't assigned to Result members. Bit in fact, they're already there! So what is actually needed is a 'type size limiting' cast, which is exactly what Result.x:=SmallInt(Result.x); is.

You are completely wrong here.... 
It will never work......

 

procedure TestSmall;
var
 lPoint : TPoint;
 sPoint : TSmallPoint;
begin
  sPoint := default(TSmallPoint);
  lpoint.x := 255;
  lpoint.y := 255;
  writeln('lPoint.x: '+lPoint.x.ToString+ '  lPoint.y: '+lPoint.y.ToString);
  sPoint := TSmallPoint(lpoint);
  writeln('sPoint.x: '+sPoint.x.ToString+ '  sPoint.y: '+sPoint.y.ToString);
end;

Compile and run.......