Decimal Type determined at Compile Time (#26) #126
Conversation
|
Passes the |
|
Oh whoops somehow I missed the email for this. I'll take a look! |
There was a problem hiding this comment.
Looks good overall, left a few comments!
And one more thing which will probably be pretty annoying, but necessary: Try compiling with -Wdouble-promotion (eg, CXXFLAGS=-Wdouble-promotion make) to find any instances where double is still being used when compiling in float mode. There are a few :) I think ultimately we'll need to wrap every decimal constant with ((decimal)180.0) for example.
|
Yeah, we might just have to. Got rid of a lot of the promotions by defining new decimal based macros for all the math stuff like M_PI. Would it be better for us to inline/define wrappers for pow and other math methods so we only need to type cast the literals? Having the decimal typecast everywhere doesn't feel right. |
|
Pushed a lot of changes regarding enforcing the usage of Ending up opting for a macro strategy in terms of enforcing decimal. You can find all of the new macros plus some more notes about them in The test suite keeps failing when running in float mode due to a weird issue (seems like a float inaccuracy issue?) regarding the basic consistency check loop. There's an example here if you want to take a look at it. I can probably fix it if I get a little guidance on the purpose of that test and what we should be expecting. Thanks! |
|
Thanks for all this! I'll take a look when I get home from work. I especially like automated tests for float and double mode in gh actions
|
src/decimal.hpp
Outdated
| #define DECIMAL_M_SQRT1_2 (decimal) M_SQRT1_2 /* 1/sqrt(2) */ | ||
|
|
||
| // Math Functions wrapped with Decimal typecast | ||
| #define DECIMAL_POW(base,power) (decimal) std::pow(base, power) |
There was a problem hiding this comment.
Do we actually need DECIMAL versions of all these functions? My understanding is that the C++ versions (eg std::pow instead of C pow from math.h) are properly overloaded to return the same type as the arguments
There was a problem hiding this comment.
Seems like it. I tried both math.h and cmath and they both cause a double promotion to occur. The other solution is to wrap every pow or abs call with the DECIMAL macro but it gets pretty unwieldy with all of the parenthesis. This solution is macro heavy but it works well for our intended use case while preserving readability.
|
About the failing test -- it's basically taking a database, running a query on it, and then checking that the results of the query all lie within the range that was requested. However, looking at it now, I'm not actually sure it's correct, because |
|
Changed it to utilize the |
src/decimal.hpp
Outdated
| // because the code becomes hard to read when there are multiple layers of typecasting. | ||
| // With this method, we might have more preprocessing to do BUT the code remains readable | ||
| // as the methods remain relatively the same. | ||
| #define DECIMAL(x) (decimal) x |
There was a problem hiding this comment.
Final comment, let's make sure all these macros are "safe" (or maybe convert them to overloaded or templated functions), for example DECIMAL(x)++ (though a very weird statement) would cause x to be incremented before being converted to decimal due to order of operations (so at minimum let's put a layer of parenthesis around the body of DECIMAL)
|
lgtm let's merge it |
For Issue #26.
Allow you to build Lost using either
floatordoubleby providing adecimaltype. Thedecimal.hppalso provides aSTR_TO_DECIMALmacro that expands tostofandstodfor floats and doubles respectively.Injects the flags (defined in
databases.hppon database serialization.Runtime DB checking on de-serialization to ensure that the decimal was the same across both.
It will use the
floattype by default but this can be changed todoubleby usingmake LOST_DATABASE_DOUBLE=1.Tested both
floatanddoublefunctionality using theimg_7760.pngexample and they both worked.