Jump to content
Fritzew

Funny Code in System.Types

Recommended Posts

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...

 

  • Confused 1
  • Sad 1

Share this post


Link to post
4 minutes ago, Mahdi Safsafi said:

Wow:


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

 

Obviously that cast is a bit pointless but it hardly matters 

Share this post


Link to post

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...

Share this post


Link to post
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!

Share this post


Link to post

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?

  • Haha 1

Share this post


Link to post
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 !

Share this post


Link to post
Posted (edited)
46 minutes ago, WillH said:

Are Emba doing any unit testing?

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

Edited by Schokohase

Share this post


Link to post
1 hour ago, Mahdi Safsafi said:

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

Er, yes, I see it now!! 

Share this post


Link to post
Posted (edited)
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 by Der schöne Günther

Share this post


Link to post
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.

Share this post


Link to post
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.

Share this post


Link to post

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...

Share this post


Link to post
Posted (edited)
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 by David Heffernan

Share this post


Link to post
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 😉

  • Like 1

Share this post


Link to post
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...

Share this post


Link to post
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...

  • Like 2

Share this post


Link to post

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.

  • Haha 1
  • Confused 1

Share this post


Link to post
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? 

Share this post


Link to post
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.......

 

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×