Use constexprs where possible #4
Conversation
|
Memory usage change @ 732a621
Click for full report table
Click for full report CSV |
In C++-11 a constexpr function may not even contain local const's. To avoid turning the implementation into an unreadable mess, I did not go for a literal "translation". Rather: - moved (an equivalent of) uFullRange() into UFix as maxRange() - moved (an equivalent of) neededNIExtra() into UFix as a typedef NIadjusted_t (returning "this type, but with NI adjusted according to RANGE) The end result actually looks quite a bit simpler to me. I hope that isn't because I'm overlooking something. NOTE: The return type of worktype::asRaw() is the same integer type as previously defined explicitly as return_type. NOTE: This is a draft, and I only converted a single operator, for now. Once finished, it should be possible to remove at least needed(S)NIExtra(), but probably also uFullRange() and sFullRange().
tfry-git
force-pushed
the
work/constexpr
branch
from
f47a13e
to
172610b
Compare
|
Memory usage change @ 172610b
Click for full report table
Click for full report CSV |
|
@tomcombriat : This is still a work in progress, but I'd like to get a first look from your side on this, before I continue to apply the same pattern for further operators:
Note that once all this is finished, I intend to implement auto-checks using static_asserts(), as outlined, some time ago (while I was still thinking, this was easy to do...). |
|
Hei! That looks quite neat! This code is quite hard to read and I think this suggestion actually makes it more readable (I doubt a bit it will ever become ultra readable anyway). I made some comments but they do not appear in the thread, in case you did not received a notifications, they are here. Looking very fast yesterday, I though that you were extensively using instead of which I remember to have found very smart as the return type can be inferred by what is returned, but I see now that you removed that. Out of curiosity, why does not this work? |
|
Memory usage change @ 485e9ff
Click for full report table
Click for full report CSV |
Replies a the same place. While addressing your points, I also went ahead and added some first static asserts, which would have immediately caught my mixup of NI and NF.
[...]
Apparently, this is not available before C++14. Compilation failed on the Uno for that reason. |
|
Memory usage change @ 8b92b62
Click for full report table
Click for full report CSV |
Autotests unveil some problems
|
Memory usage change @ a780467
Click for full report table
Click for full report CSV |
|
Ok, so here's the complete PR. We could certainly add a lot more static checks, but I think it's a start. While doing this, I ran into two issues:
|
|
Wow, nice! I'll try to have a closer look at it very soon! Regarding the issues:
No, this is because of the way the inverse is computed. Let's take a
|
There was a problem hiding this comment.
Looks good to me! No testing done but I am working these days on a code that uses FixMath extensively so a big mistake won't stay unnoticed for very long.
I added a few comments here and there, mostly to make sure I understand, so feel free to ignore them.
Once merged, I suggest waiting a couple of weeks to let an obvious bug show its face and then make a new release with that!
I agree that the code looks quite neater! Thanks!
| // multiplication | ||
| static_assert(c * UFix<36, 5>(3ll << 31) == UFix<58,0>(33ll*(3ll << 31))); // NOTE: The exact values are aribrary, but we want something that would overflow the initial type range | ||
| static_assert(a * UFix<0, 2>(3, true) == UFix<17, 8>(24)); // 32 * .75 == 24 | ||
| // static_assert(a * UFix<5, 0>(4).invAccurate() == UFix<17, 8>(8)); // 32 * (1/4) == 8 // TODO: FAIL. Isn't this supposed to work? |
There was a problem hiding this comment.
See my comment in the thread. This is a limitation because of the way it is computed. Note that we know exactly how far it can be (hence do a static_assert with a range of the output instead of a fixed value).
There was a problem hiding this comment.
Ok, I see that now. That cues two further questions (neither of which I'd intend to address within this PR):
- Maybe add a convenience function for ranged comparisons?
- I see what you mean, now, but invAccurate() sounded like - it would be accurate... I guess that's mostly a naming thing. If you don't deem it too late to change that, I'd suggest renaming as perhaps invFull()? And maybe a new function that works closer to what I expected would have a value, too (for small numbers, the difference can become quite noticeable).
There was a problem hiding this comment.
If you don't deem it too late to change that, I'd suggest renaming as perhaps invFull()?
Sounds like a good idea! I will try to change that (and add a real accurate) soon. Note that in all cases, if the value cannot be represented in the destination type well, you are done for… If find a bit fun that division are the only operations which lose precision in fixed point math!
I think now is the time for these kind of things, I think I used one or two invAccurate in Mozzi (where speed is usually more important than precision) that can easily be changed into invFull
Compilers _might_ turn this into smarter code that e.g. will not check all bytes for equality, if a first byte mismatches.
|
Memory usage change @ 281fde9
Click for full report table
Click for full report CSV |
Work in progress.
Notes to self: