Flow enum

[seelabs]

Refactoring some payment code (this will be used in another w.i.p. PR, but since it's small and stands on it's own I gave it its own PR). There should be no functional changes in these patches.

A couple notes:

1. The splitting qualities into qualitiesSrcRedeems and qualitiesSrcIssues is needed in the w.i.p. PR. Otherwise I would have kept the single function.

2. I added a commit for convenience function for one of the enums. There's always a tradeoff for small functions like this. I left it separate so it'll be easy to remove if reviewers don't like it.

[ripplelabs-jenkins]

Jenkins Build Summary

Built from this commit

Built at 20190802 - 23:24:31

Test Results

Build Type	Log	Result	Status
msvc.Debug	logfile	1178 cases, 0 failed, t: 570s	PASS ✅
msvc.Debug, NINJA_BUILD=true	logfile	1178 cases, 0 failed, t: 609s	PASS ✅
msvc.Debug -Dunity=OFF	logfile	1177 cases, 0 failed, t: 1105s	PASS ✅
msvc.Release	logfile	1178 cases, 0 failed, t: 403s	PASS ✅
gcc.Release -Dassert=ON, MANUAL_TESTS=true	logfile	915 cases, 0 failed, t: 5m0s	PASS ✅
gcc.Debug -Dcoverage=ON, TARGET=coverage_report, SKIP_TESTS=true	logfile	1178 cases, 0 failed, t: 16m49s	PASS ✅
docs, TARGET=docs	logfile	1 cases, 0 failed, t: 0m1s	PASS ✅
clang.Debug	logfile	1178 cases, 0 failed, t: 2m54s	PASS ✅
clang.Debug -Dunity=OFF	logfile	1177 cases, 0 failed, t: 10m57s	PASS ✅
gcc.Debug	logfile	1178 cases, 0 failed, t: 2m55s	PASS ✅
gcc.Debug -Dunity=OFF	logfile	1177 cases, 0 failed, t: 11m28s	PASS ✅
clang.Release -Dassert=ON	logfile	1178 cases, 0 failed, t: 5m8s	PASS ✅
gcc.Release -Dassert=ON	logfile	1178 cases, 0 failed, t: 4m58s	PASS ✅
gcc.Debug -Dstatic=OFF	logfile	1178 cases, 0 failed, t: 2m56s	PASS ✅
gcc.Debug -Dstatic=OFF -DBUILD_SHARED_LIBS=ON	logfile	1178 cases, 0 failed, t: 2m56s	PASS ✅
gcc.Debug, NINJA_BUILD=true	logfile	1178 cases, 0 failed, t: 2m49s	PASS ✅
clang.Debug -Dunity=OFF -Dsan=address, PARALLEL_TESTS=false, DEBUGGER=false	logfile	1177 cases, 0 failed, t: 11m29s	PASS ✅
clang.Debug -Dunity=OFF -Dsan=undefined, PARALLEL_TESTS=false	logfile	1177 cases, 0 failed, t: 12m36s	PASS ✅

[scottschurr]

I'm just getting started on this review, but I wanted to leave some first impressions.

1. I 😍 the type safety that the enums provide. This is a huge step up for maintainability. Thanks!

2. I don't think the change has hurt readability, but I'm wondering if it can be improved a bit from where we're at.

On that last point, here's what I'm thinking... All three of the new enums are named *Direction. And as parameters they tend to be named dir. That means when I'm reading source and I see dir I don't know which of the three kinds I'm reading.

So here are alternative names for the enums for you to consider.

enum class IssuesOrRedeems { issues, redeems }; // parameter name issuesOrRedeems
enum class QualityInOrOut { in, out };          // parameter name inOrOut
enum class StrandFwdOrRev { forward, reverse }; // parameter name fwdOrRev

I'm not great at naming, so I'm not wedded to these names. But I think they may be easier to distinguish than the ones you're currently using. I'm sure there are yet better names.

Just for your consideration.

Thanks for doing this!

[seelabs]

Let's look at how changing the variables type DebtDirection to IssuesOrRedeems and variable named dir to issuesOrRedeems effects the code.

Type declarations:

From:

qualityUpperBound(ReadView const& v, DebtDirection& dir) const override;

To:

qualityUpperBound(ReadView const& v, IssuesOrRedeems& issuesOrRedeems) const override;

From:

    bool verifyPrevStepDebtDirection (DebtDirection prevStepDir) const
    {
        return issues(prevStepDir);
    }

To:

    bool verifyPrevStepDebtDirection (IssuesOrRedeems prevStepIssuesOrRedeems) const
    {
        return issues(prevStepIssuesOrRedeems);
    }

(Or if we don't use the small helper functions)

From:

    bool verifyPrevStepDebtDirection (DebtDirection prevStepDir) const
    {
        return prevStepDir == DebtDirection::issues;
    }

To:

    bool verifyPrevStepDebtDirection (IssuesOrRedeems prevStepIssuesOrRedeems) const
    {
        return prevStepIssuesOrRedeems == DebtDirection::issues;
    }

From:

    auto const prevStepDebtDir = dir;
    dir = this->debtDirection(v, StrandDirection::forward);
    std::uint32_t const srcQOut = [&]() -> std::uint32_t {
        if (redeems(prevStepDebtDir) && issues(dir))
            return transferRate(v, src_).value;
        return QUALITY_ONE;
    }();

To:

    auto const prevStepIssuesOrRedeems = issuesOrRedeems;
    issuesOrRedeems = this->issuesOrRedeems(v, StrandDirection::forward);
    std::uint32_t const srcQOut = [&]() -> std::uint32_t {
        if (redeems(prevStepIssuesOrRedeems) && issues(dir))
            return transferRate(v, src_).value;
        return QUALITY_ONE;
    }();

I prefer the "From" version in all those cases. Given where a variable like dir is used, there usually a clue on the line that shows what kind of direction it is (i.e. dir = this->debtDirection(v, StrandDirection::forward); that dir is the result of a function call to debtDirection, so it's not going to be a StrandDirection or QualityDirection. Or there is a type name next to it i.e. (DebtDirection& dir) others are similar).

Having said all that, I don't love the name DebtDirection. However, I do like it better than IssuesOrRedeems. I'll try to think of a better name;

[jwbusch]

👍, bool input parameters being replaced by enums always makes me happy.

[scottschurr]

Looks great to me. 👍

[seelabs]

Thanks for the reviews: Squashed, forced-pushed, and parked passed (tho of course I'll respond to more feedback if it comes)