Inline functions

[darbyjohnston]

Fixes #996

Currently a number of the C++ headers have the function declarations combined with the definitions, i.e.:

class Foo
{
public:
    void print_hello_world()
     {
         std::cout << "hello world!" << std::endl;
     }
};

As a C++ developer it would be nicer to browse the class API if the function definitions were separated, i.e.:

class Foo
{
public:
    void print_hello_world();
};

inline void Foo::print_hello_world()
{
    std::cout << "hello world!" << std::endl;
}

This example is trivial but with large classes with a lot of implementation code it can obscure the class API. OpenEXR does this fairly consistently:
https://github.com/AcademySoftwareFoundation/openexr/blob/master/src/lib/OpenEXR/ImfChannelList.h

The code changes would be cosmetic only, no functionality would be changed. I'll submit a PR to show what this looks like for a couple of header files, if the changes sound reasonable I can update the rest of them. However if this seems like too large of a change I'm also fine to close this issue.

[codecov-commenter]

Codecov Report

Merging #997 (d532fd0) into master (d33bf24) will decrease coverage by 0.00%.
The diff coverage is 88.40%.

@@            Coverage Diff             @@
##           master     #997      +/-   ##
==========================================
- Coverage   85.60%   85.60%   -0.01%     
==========================================
  Files         192      192              
  Lines       18331    18336       +5     
  Branches     2056     2056              
==========================================
+ Hits        15693    15696       +3     
- Misses       2112     2114       +2     
  Partials      526      526              

Flag	Coverage Δ
unittests	85.60% <88.40%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/opentimelineio/serializableObject.cpp	68.67% <80.00%> (+1.55%)	⬆️
src/opentime/rationalTime.h	87.50% <87.50%> (+0.26%)	⬆️
src/opentimelineio/serializableObject.h	87.95% <90.00%> (-0.94%)	⬇️
src/opentimelineio/imageSequenceReference.cpp	94.25% <0.00%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d33bf24...d532fd0. Read the comment docs.

[meshula]

This change highlights another issue related to the very notion of inlining.

I hadn't considered this point previously, but reading the diff made me think about what was being split, and what impact it might have on code generation.

Our methods are marked const, but the majority of things marked const should be marked constexpr. Also pretty much every function in these two headers should be marked noexcept. I just added a new issue. #999

As a style point, inlines as we have them look fine to me, and splitting things out as you propose looks fine to me, so I'll defer the style question to others.

[darbyjohnston]

I don't think this change would have an impact on code generation, but that's a good point about noexcept/constexpr, I'd be happy to make a pass at that in a separate PR.

[darbyjohnston]

So, does this seem like a good idea? If so I would like to un-draft it, otherwise I'll close it and the corresponding issue:
#996

[meshula]

This is a stylistic change that I don't feel strongly about one way or another. My bias is that it looks easier to read as a user of the API, and harder to maintain as a developer of the API, mostly because I don't use this pattern myself because it's harder to visually spot missing implementations; not really an issue here, given the relative maturity of the interface.

Perhaps someone else has some thoughts.