Port number's mantissa to std::vector by janisozaur · Pull Request #544 · microsoft/calculator

janisozaur · 2019-06-06T22:17:14Z

These are the changes extracted from #211 that port number's mantissa
from flexible member arrays, a non-standard C++ extension, to
std::vector, which enables compilation and linkage with clang.

@fwcd is the original author of these changes, I (@janisozaur) merely
rebased them on top of current master.

The rebase was luckily fairly straight-forward, with the only real
conflict coming from 7a7ceb5
(#412), but was trivial to
fix.

mcooley · 2019-06-07T00:28:04Z

src/CalcManager/Ratpack/conv.cpp


    // Loop over all the relative digits from MSD to LSD
-    for (ptr = &(a->mant[a->cdigit - 1]); cdigits > 0; ptr--, cdigits--)
+    for (ptr = &(a.mant[a.cdigit - 1]); cdigits > 0; ptr--, cdigits--)


The unit tests are failing, and I think the problem is here--we're calling a.mant[-1].

One reasonable fix would be to rewrite this as a while loop so we check cdigits first:

while(cdigits > 0)
{
ptr = &(a.mant[a.cdigit - 1]);
// etc.
ptr--;
cdigits--;
}

janisozaur · 2019-06-10T20:57:57Z

I rebased this on top of already approved #545 to preempt minor conflict and fixed some issues I found. This might address the warnings raised previously, I'd like to hear back from the test suite, would be nice to have it fixed

mcooley · 2019-06-10T21:42:19Z

/azp run

azure-pipelines · 2019-06-10T21:42:24Z

Pull request contains merge conflicts.

mcooley · 2019-06-10T22:09:14Z

Sorry for the test pipeline outage earlier. The unit tests are still failing with the same issue, unfortunately.

janisozaur · 2019-06-11T21:30:33Z

Judging by the error message reported, the tests are failing because of infrastructure still.

artifact-engine: unhandled rejection Failed to extract package with error Error: ENOENT: no such file or directory, open 'D:\_work\1\a\drop.zip'

mcooley · 2019-06-11T22:33:14Z

/azp run

azure-pipelines · 2019-06-11T22:33:27Z

Azure Pipelines successfully started running 1 pipeline(s).

mcooley · 2019-06-12T14:07:45Z

/azp run

azure-pipelines · 2019-06-12T14:08:01Z

Azure Pipelines successfully started running 1 pipeline(s).

mcooley · 2019-06-12T15:03:05Z

/azp run

azure-pipelines · 2019-06-12T15:03:35Z

Azure Pipelines successfully started running 1 pipeline(s).

mcooley · 2019-06-12T16:08:24Z

Still working on this CI infrastructure issue--I'm waiting for another team to investigate.

I tried running the unit tests locally, and I'm still seeing failures. The next one I see is a vector subscript out of range in mulnumx, line 42. There wasn't an obvious logic change there, so a deeper investigation will be needed.

janisozaur · 2019-06-12T17:27:24Z

How can I get size of mantissa in its previous (flexible array) form? The equivalent of num.mant.size() in the new form? What is the value of num->mant[0] in previous form, when default-initialised? Is that a legal access?

janisozaur · 2019-06-12T21:19:42Z

src/CalcManager/Ratpack/conv.cpp

-        throw(CALC_E_INVALIDRANGE);
-    }
-    return (pnumret);
+    NUMBER num = NUMBER();


This would create a default-constructed value, which will have sign equal 1:

calculator/src/CalcManager/Ratpack/ratpak.h

Line 63 in 63e4a12

int32_t sign = 1; // The sign of the mantissa, +1, or -1

That's unlike the current/old code, which would initialise it to 0 (via zmalloc call, which calls calloc underneath).

janisozaur · 2019-06-12T21:27:13Z

src/CalcManager/Ratpack/conv.cpp

    {
        // Remove them.
-        memmove(pnum->mant, pmant, (int)(cdigits * sizeof(MANTTYPE)));
+        copy(pmant, pmant + cdigits, pnum.mant.begin());


In case the elements overlap, the result of std::copy is undefined, unlike with memmove

janisozaur · 2019-06-12T21:30:13Z

src/CalcManager/Ratpack/num.cpp

+    if (c.mant.begin() != ++ptrc)
    {
-        destroynum(num);
+        copy(ptrc, ptrc + cdigits, c.mant.begin());


memmove vs std::copy again

janisozaur · 2019-06-12T21:39:16Z

src/CalcManager/Ratpack/ratpak.h

+                        // radix being used.
+    int32_t exp = 0;    // The offset of digits from the radix point
+                        // (decimal point in radix 10)
+    std::vector<MANTTYPE> mant;


It seems cdigit is used as count of entries in the flexible array in the long form, is that correct? Additionally, cdigits usage seems to suggest such arrays used to have at least one element defaulting to zero, is that correct? Since vector by default doesn't contain any values, perhaps default-initialising it with a single zero would address the current problems:

Suggested change

std::vector<MANTTYPE> mant;

std::vector<MANTTYPE> mant{0};

janisozaur · 2019-06-12T21:40:08Z

@mcooley I highlighted some possible differences, could you review them?

mcooley · 2019-06-13T06:58:20Z

/azp run

azure-pipelines · 2019-06-13T06:58:37Z

Azure Pipelines successfully started running 1 pipeline(s).

mcooley · 2019-06-13T16:05:36Z

/azp run

azure-pipelines · 2019-06-13T16:05:50Z

Azure Pipelines successfully started running 1 pipeline(s).

mcooley · 2019-06-13T16:46:07Z

Okay, looks like we’ve finally worked around the CI infrastructure issues.

Thanks for flagging all these spots to double-check. I am about to head out on vacation for a few days, but I will take a closer look at these next week.

janisozaur · 2019-06-13T17:44:25Z

@mcooley is there any way I can get Azure to produce actual error messages, instead of the rather unhelpful

Error Message:
 Exception Code: C0000005

?

AjayVV · 2019-06-19T18:30:42Z

src/CalcManager/CEngine/Number.cpp

    {
-        m_mantissa.reserve(p->cdigit);
-        copy(p->mant, p->mant + p->cdigit, back_inserter(m_mantissa));
+        m_mantissa.reserve(p.cdigit);


Why not use resize and thence avoid back_inserter ?

janisozaur · 2019-06-20T19:11:20Z

All the CI runs are green now! @mcooley can you review now?

danbelcher-MSFT

Thanks for getting this working! My main concern with this change is around performance and clarity of the codebase. Previously we were passing and manipulating everything as PNUMBER and now we've switched to NUMBER. In some cases, there is no performance loss, such as changing from PNUMBER to NUMBER &, but I'm generally concerned that we may have introduced additional copies etc that weren't there previously. There are a number of places where PNUMBER was changed to NUMBER* but could have been changed to NUMBER& for increased safety (I recognize it's a cascading change that is outside the scope of your work here, but it's worth bringing up). There's also some confusion in the codebase in the distinction between Number and NUMBER, as well as the naming of data members (pq is not a pointer). Number is intended to be the modern replacement for the NUMBER POD. Hopefully we see the codebase start to move functionality into the Number/Rational classes and we can drop NUMBER/RAT altogether.

danbelcher-MSFT · 2019-06-21T03:56:57Z

src/CalcManager/CEngine/Number.cpp

-    Number::Number(PNUMBER p) noexcept
-        : m_sign{ p->sign }
-        , m_exp{ p->exp }
+    Number::Number(NUMBER p) noexcept


Suggested change

Number::Number(NUMBER p) noexcept

explicit Number::Number(const NUMBER& p) noexcept

danbelcher-MSFT · 2019-06-21T03:58:02Z

src/CalcManager/CEngine/Number.cpp

    }

-    PNUMBER Number::ToPNUMBER() const
+    NUMBER Number::ToNUMBER() const


Suggested change

NUMBER Number::ToNUMBER() const

explicit Number::operator NUMBER() const

danbelcher-MSFT · 2019-06-21T04:16:33Z

src/CalcManager/CEngine/Number.cpp

-            *ptrRet++ = digit;
-        }
-
+        NUMBER ret = _createnum(static_cast<uint32_t>(this->Mantissa().size()) + 1);


Can we get rid of _createnum and use a constructor instead?

danbelcher-MSFT · 2019-06-21T04:31:16Z

src/CalcManager/Ratpack/conv.cpp

        // Mantissa specified, convert to number form.
-        PNUMBER pnummant = StringToNumber(mantissa, radix, precision);
-        if (pnummant == nullptr)
+        optional<NUMBER> pnummant = StringToNumber(mantissa, radix, precision);


std::optional works well with auto and pointer semantics:

Suggested change

optional<NUMBER> pnummant = StringToNumber(mantissa, radix, precision);

auto pnummant = StringToNumber(mantissa, radix, precision);

if (!pnummant)

{

...

}

danbelcher-MSFT · 2019-06-21T04:32:06Z

src/CalcManager/Ratpack/conv.cpp

    }

-    if (round != nullptr)
+    if (round.has_value())


Suggested change

if (round.has_value())

if (round)

danbelcher-MSFT · 2019-06-21T04:34:27Z

src/CalcManager/Ratpack/num.cpp

-            destroynum(tmp);
            tmp = lasttmp;
-            lasttmp = nullptr;
+            lasttmp = optional<NUMBER>();


Suggested change

lasttmp = optional<NUMBER>();

lasttmp = nullopt;

danbelcher-MSFT · 2019-06-21T04:40:16Z

src/CalcManager/Ratpack/ratpak.h

 {
-    PNUMBER pp;
-    PNUMBER pq;
+    NUMBER pp;


Suggested change

NUMBER pp;

NUMBER p;

NUMBER q;

The names pp and pq are shorthand for pointer to p and pointer to q. There will be a lot of change in the codebase if you rename these, but we're also adding confusion now that the names have embedded type information that isn't actually correct.

danbelcher-MSFT · 2019-06-21T04:42:16Z

src/CalcManager/Ratpack/ratpak.h

-        OutputDebugString(outputString.str().c_str());                                                                                                         \
-    }                                                                                                                                                          \
-    _destroynum(x), (x) = nullptr
+{


I don't think this is actually valid?

janisozaur · 2019-08-02T21:12:07Z

I've rebased it and updated to include changes required after #498, but haven't addressed the review comments yet.

janisozaur · 2019-08-15T07:42:17Z

I started writing some benchmarks using google benchmark, but have never really finished them to a point where they would be worth submitting and eventually lost interest in that, as other things took my time away. I can't say I fully agree with the argument about possible performance loss, when there is no benchmark available in the upstream project, so I will simply address the review comments as they are now.

@fwcd

These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.

mcooley · 2020-03-13T16:21:28Z

Closing this since it's become stale and we haven't had time to thoroughly investigate the regressions in our primary environment (Visual Studio on Windows). Thanks for your interest in working on portability; we hope that some of these changes can be contributed in smaller pieces in the future.

janisozaur · 2021-10-16T21:55:10Z

I've rebased the code here to a branch 'mantissa-202110' in my fork. This is currently under testing, hopefully will submit another PR at some point.

mcooley reviewed Jun 7, 2019

View reviewed changes

HowardWolosky assigned mcooley Jun 10, 2019

HowardWolosky added the codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) label Jun 10, 2019

janisozaur force-pushed the mantissa branch from 8331f76 to fd4c594 Compare June 10, 2019 20:52

janisozaur force-pushed the mantissa branch 2 times, most recently from 5b5046f to ce8d263 Compare June 11, 2019 21:12

janisozaur commented Jun 12, 2019

View reviewed changes

AjayVV reviewed Jun 19, 2019

View reviewed changes

janisozaur force-pushed the mantissa branch 2 times, most recently from dc1e229 to 000733d Compare June 20, 2019 18:49

danbelcher-MSFT suggested changes Jun 21, 2019

View reviewed changes

janisozaur mentioned this pull request Jul 8, 2019

Fix #563: Odd dependency cycle #570

Merged

janisozaur force-pushed the mantissa branch from 000733d to 95127a5 Compare August 2, 2019 21:08

janisozaur force-pushed the mantissa branch 2 times, most recently from 6b39c64 to 9d0cec9 Compare December 10, 2019 22:32

fwcd and others added 7 commits December 10, 2019 23:33

Restore usage of SIGN macro

2c522c5

Remove unused code

1419da1

Restore initialization of constants

7c8d821

Fix stripzeroesnum arguments passing

3bf94e2

Update to address changes from microsoft#498

510823e

Rebase fixes

80162ad

janisozaur force-pushed the mantissa branch from 9d0cec9 to 80162ad Compare December 10, 2019 22:34

mcooley closed this Mar 13, 2020

	Number::Number(NUMBER p) noexcept
	explicit Number::Number(const NUMBER& p) noexcept

	NUMBER Number::ToNUMBER() const
	explicit Number::operator NUMBER() const

-        optional<NUMBER> pnummant = StringToNumber(mantissa, radix, precision);
+        auto pnummant = StringToNumber(mantissa, radix, precision);
+        if (!pnummant)
+        {
+            ...
+        }

Conversation

janisozaur commented Jun 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janisozaur commented Jun 10, 2019

Uh oh!

mcooley commented Jun 10, 2019

Uh oh!

azure-pipelines bot commented Jun 10, 2019

Uh oh!

mcooley commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janisozaur commented Jun 11, 2019

Uh oh!

mcooley commented Jun 11, 2019

Uh oh!

azure-pipelines bot commented Jun 11, 2019

Uh oh!

mcooley commented Jun 12, 2019

Uh oh!

azure-pipelines bot commented Jun 12, 2019

Uh oh!

mcooley commented Jun 12, 2019

Uh oh!

azure-pipelines bot commented Jun 12, 2019

Uh oh!

mcooley commented Jun 12, 2019

Uh oh!

janisozaur commented Jun 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janisozaur commented Jun 12, 2019

Uh oh!

mcooley commented Jun 13, 2019

Uh oh!

azure-pipelines bot commented Jun 13, 2019

Uh oh!

mcooley commented Jun 13, 2019

Uh oh!

azure-pipelines bot commented Jun 13, 2019

Uh oh!

mcooley commented Jun 13, 2019

Uh oh!

janisozaur commented Jun 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janisozaur commented Jun 20, 2019

Uh oh!

danbelcher-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janisozaur commented Aug 2, 2019

Uh oh!

janisozaur commented Aug 15, 2019

mcooley commented Jun 10, 2019 •

edited

Loading