View Issue Details

IDProjectCategoryView StatusLast Update
0037275FPCRTLpublic2020-06-28 16:21
ReporterThaddy de Koning Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
PlatformallOSall 
Product Version3.3.1 
Summary0037275: patch: add comparevalue for currency to math
Descriptioncomparevalue is missing an overload for currency.
see discussion here: https://forum.lazarus.freepascal.org/index.php/topic,50358.msg367281.html#msg367281
Steps To ReproduceSee discussion
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Activities

Thaddy de Koning

2020-06-27 10:32

reporter  

comparevalue.patch (977 bytes)   
Index: rtl/objpas/math.pp
===================================================================
--- rtl/objpas/math.pp	(revision 45658)
+++ rtl/objpas/math.pp	(working copy)
@@ -624,7 +624,9 @@
 function CompareValue ( const A, B  : Integer) : TValueRelationship; inline;
 function CompareValue ( const A, B  : Int64) : TValueRelationship; inline;
 function CompareValue ( const A, B  : QWord) : TValueRelationship; inline;
+function CompareValue(const A, B  : Currency): TValueRelationship;inline;
 
+
 {$ifdef FPC_HAS_TYPE_SINGLE}
 function CompareValue ( const A, B : Single; delta : Single = 0.0 ) : TValueRelationship; inline;
 {$endif}
@@ -2530,6 +2532,16 @@
      result:=LessThanValue;
 end;
 
+function CompareValue(const A, B  : Currency): TValueRelationship;
+begin
+  result:=GreaterThanValue;
+  if a=b then
+    result:=EqualsValue
+  else
+   if a<b then
+     result:=LessThanValue;
+end;
+
 function CompareValue(const A, B: Int64): TValueRelationship;
 
 begin
comparevalue.patch (977 bytes)   

Thaddy de Koning

2020-06-27 10:33

reporter   ~0123612

Last edited: 2020-06-27 10:41

View 2 revisions

Plz check if the patch is correct. I am not quite sure I added the overload in the right place...
But it is tested on debian and windows.

Serge Anvarov

2020-06-27 12:27

reporter   ~0123613

It should be noted that the problem is only for 64x in Windows. In Win64, the Currency type is mapped to Int64, but it is a float type, and the compiler cannot choose between Int64 and Double. In Win32, there are no problems - a function with the Extended type is selected (as in Delphi). But FPC Win64 does not provide version with Extended.
The provided solution depends on the current Currency type representation in Win64. If it is changed (from Int64), the solution may become incorrect.

Thaddy de Koning

2020-06-27 13:44

reporter   ~0123616

Last edited: 2020-06-27 13:46

View 3 revisions

@ASerge
The problem exists on *all* platforms so that conclusion is wrong. It is not a x86_64 issue.
Tested arm32 (Raspbian) first, after I wrote the patch also intel64 debian and windows.
On all these three the problem shows up.

Serge Anvarov

2020-06-27 14:52

reporter   ~0123618

On Win32 no problem, as I mentioned. That is, the expression "all" is not quite accurate here.

Thaddy de Koning

2020-06-27 19:11

reporter   ~0123630

I can't test win32. (well, will not test, I tested a 32 bit arm though)
Anyway: the patch should solve it for all platforms that support type currency. (I supposed that all do, since it is not a float)

Sven Barth

2020-06-27 22:31

manager   ~0123635

@jamie: your local CompareValue overload does not have the overload directive, thus the compiler will stop looking any further (e.g. it will also use the local Currency overload for Double values). If you add the overload directive or the overload is in the same unit as the others (in this case the Math unit) then it will work correctly.

@topic: The general problem is that the compiler has to decide between the Single and the Double overload (all others are irrelevant for this). Delphi correctly picks the Double overload if only those two are available, but FPC does not.

jamie philbrook

2020-06-27 22:48

reporter   ~0123638

The overloading issue has been an ass ache for far too long and it seems the DEV are more interested in putting new features, there by generating more bugs over fixing the bugs there now..

 I have several issues with the compiler picking the wrong function due to operations that should work but they don't. all of this works fine in Delphi and has for years.

 Oh well...
    My boss was right

I'll erase my contribution to this if I can figure it out, most likely another tag scoping issue will stop me from that, too.

Sven Barth

2020-06-28 00:40

manager   ~0123642

Delphi uses the same approach to cross unit overloading. In FPC it is simply more visible, because FPC does not require the use of the overload directive to allow overloading between functions of the same unit. In hindsight this might be considered problematic, but it is how it is now.

Also I've just now fixed the correct selection of functions for Currency if the compiler needs to decide between Single and Double. Fixing bugs for the overload selection is a very delicate affair however as one might easily break other, correct code. Not to mention that it's hard to find the correct rules for everything, because the minute details are not documented.

That said in most cases FPC's overload selection is more precise than Delphi's as FPC uses a more sophisticated algorithm. This might lead to different results than for Delphi, but more often than not correct ones.

@topic: with the underlying issue fixed in r45707 the question is whether we still want to add that Currency overload...

Bart Broersma

2020-06-28 16:21

reporter   ~0123653

Well, I favor explicit over implicit rules (as they are easier to understand from the end users point of view), so yes: add it.

Issue History

Date Modified Username Field Change
2020-06-27 10:32 Thaddy de Koning New Issue
2020-06-27 10:32 Thaddy de Koning File Added: comparevalue.patch
2020-06-27 10:33 Thaddy de Koning Note Added: 0123612
2020-06-27 10:41 Thaddy de Koning Note Edited: 0123612 View Revisions
2020-06-27 12:27 Serge Anvarov Note Added: 0123613
2020-06-27 13:44 Thaddy de Koning Note Added: 0123616
2020-06-27 13:45 Thaddy de Koning Note Edited: 0123616 View Revisions
2020-06-27 13:46 Thaddy de Koning Note Edited: 0123616 View Revisions
2020-06-27 14:52 Serge Anvarov Note Added: 0123618
2020-06-27 19:11 Thaddy de Koning Note Added: 0123630
2020-06-27 22:31 Sven Barth Note Added: 0123635
2020-06-27 22:48 jamie philbrook Note Added: 0123638
2020-06-28 00:40 Sven Barth Note Added: 0123642
2020-06-28 16:21 Bart Broersma Note Added: 0123653