C++ headers with separate declarations/definitions

[darbyjohnston]

Fixes #996

This PR separates C++ inline functions from class declarations. This is a stylistic change to help C++ developers browse the class API without having to see implementation details. The inline functions are moved out of the class to the bottom of the header file where they can still be viewed if desired. OpenEXR uses a similar pattern:
https://github.com/AcademySoftwareFoundation/openexr/blob/main/src/lib/OpenEXR/ImfChannelList.h

[reinecke]

After the TSC meeting discussion, next steps:

1. Let's see how different IDEs handle this (jump to definition vs. jump to declaration) - do they behave as expected?
2. Let's go ahead and see what doxygen docs look like

Once we get this as a wholistic example we like, we can merge as the example and then everyone to contribute to conforming the codebase over time.

[meshula]

Unfortunately the follow on steps 1 and 2 never occurred. I am not an advocate of this style; and with no other input and a year of inactivity I have to recommend closing, with apologies.

[darbyjohnston]

Unfortunately the follow on steps 1 and 2 never occurred

I would like to work on those, but I didn't want to stack additional changes on top of the C++17 PR.

I am not an advocate of this style

Can you say why not?

[meshula]

Please read this as trying to be constructive, rather just taking an arbitrary position about stylistic choices. To me, this is not a stylistic change, it's a productivity change, so my critique would be about why I think this change should introduce some productivity engineering. Let me explain.

random observations

This is a stylistic change to help C++ developers browse the class API without having to see implementation details.

For me, emphasis on "me", that's not a benefit. I tend work ON otio, more than WITH otio, so for me, this is a burden of "is the implementation somewhere in this header file or is it in the cpp". Since the API declaration doesn't include a hint about being inline, it's extra load. consider is_valid_time; it's now inline, but there's no hints such as "constexpr" to tell me it's in the header.

my next observation is that the moved functions are trivial for the most part

constexpr double RationalTime::value_rescaled_to(double new_rate) const noexcept
 {
     return new_rate == _rate ? _value : (_value * new_rate) / _rate;
 }

 constexpr double RationalTime::value_rescaled_to(RationalTime rt) const noexcept
 {
     return value_rescaled_to(rt._rate);
 }

so if my question is "what does value_rescaled_to" do, I can no longer look at the declaration, I have to search for the implementation (is it in the header? the cpp?) and then discover the functionality. Maybe others don't rely on reading the code, but I trust the code more than a prose description in a comment, which we don't have anyway at all, shame on us.

For slightly non-trivial ones, eg

inline RationalTime const& RationalTime::operator+=(RationalTime other) noexcept
{
    if (_rate < other._rate)
    {
        _value = other._value + value_rescaled_to(other._rate);
        _rate  = other._rate;
    }
    else
    {
        _value += other.value_rescaled_to(_rate);
    }
    return *this;
}

I still feel there's more value in knowing at a glance that (1) the result will be rescaled to the highest of the two rates, and that (2) there is no optimized case for what if the rates are equal (why not, are we deferring everything to compiler magic? I don't think that's the intent so it's probably an oversight). So I already got a lot of useful information by glancing at the operator; out of sight is out of mind....

Finally, we don't have documentation to refer to particularly for C++, so I can't say that I use the documentation to discover the API, but I do use auto completion, even in vim, so if I want to discover the API I don't discover it from the header, I discover it via text editor functionality.

So, this is just me, and I don't speak for anyone but myself, but as is this pattern doesn't work for me.

a proposal

So this gets to the part where I propose that maybe this could be an important productivity change instead of nominally a stylistic change.

The thing that would change me mind is if the headers were properly documented. For example, += explained the behavior, in the header, as I did above, not by winking in the direction of the python doc strings, AND the unit tests give confidence that the behavior is as documented (I think they do for the most part), I'd be much happier about it.

So for me, I'd be swayed into camp "separate" if a PR included

1. thorough doxygenation of at least the factored functions
2. in line documentation describes the behavior, even if it's trivial it should say the trivial thing
3. unit tests are double checked in each case that the behavior described in the documentation are accounted for.

Even though I request doxygen, I also don't expect that we also have to immediately add a doxygen build step. I never build doxygen docs personally, but do appreciate the uniformity in header files, and the fact that "even" vim understands them in a helpful way with little effort.

[darbyjohnston]

Fair points, and I think that's an important distinction between a developer and user of the code. As a user of the code I want to look at the API and not have to worry about the the implementation; I trust the developers did a good job and I am just trying to make use of that work. Especially as a new user, having the API and implementation mixed together adds complexity that can make it more difficult to understand.

This approach does add some extra burden to the developers, but I think it's worthwhile if it makes the user experience better. Especially since there will hopefully be many more users than developers.

I totally agree with all the points in your proposal, I was hoping this was one step along the way. How about a separate Doxygen PR first and then we can revisit this one?

[meshula]

Sure! BTW I'm perfectly fine with doxygenating just this one file to establish a pattern ~ I think that is what you are proposing.

[darbyjohnston]

@meshula I created a draft PR for adding more Doxygen documentation, starting with RationalTime:
#1746

It would be great to get your input on it.