clang notices that constexpr can't be used on a mutating function

[meshula]

This PR fixes an issue raised by the version of Clang bundled with XCode 13.x, see below. Clang correctly notices that constexpr can't be used on a mutating function.

Users/nporcino/build/local/include/opentime/rationalTime.h:160:20: error: cannot assign to non-static data member within const member function 'operator+='
            _value = other._value + value_rescaled_to(other._rate);
            ~~~~~~ ^
/Users/nporcino/build/local/include/opentime/rationalTime.h:156:35: note: member function 'opentime::v1_0::RationalTime::operator+=' is declared const here
    constexpr RationalTime const& operator+=(RationalTime other) noexcept
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/nporcino/build/local/include/opentime/rationalTime.h:161:20: error: cannot assign to non-static data member within const member function 'operator+='
            _rate  = other._rate;
            ~~~~~  ^
/Users/nporcino/build/local/include/opentime/rationalTime.h:156:35: note: member function 'opentime::v1_0::RationalTime::operator+=' is declared const here
    constexpr RationalTime const& operator+=(RationalTime other) noexcept
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/nporcino/build/local/include/opentime/rationalTime.h:165:20: error: cannot assign to non-static data member within const member function 'operator+='
            _value += other.value_rescaled_to(_rate);
            ~~~~~~ ^
/Users/nporcino/build/local/include/opentime/rationalTime.h:156:35: note: member function 'opentime::v1_0::RationalTime::operator+=' is declared const here
    constexpr RationalTime const& operator+=(RationalTime other) noexcept
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/nporcino/build/local/include/opentime/rationalTime.h:174:20: error: cannot assign to non-static data member within const member function 'operator-='
            _value = value_rescaled_to(other._rate) - other._value;
            ~~~~~~ ^
/Users/nporcino/build/local/include/opentime/rationalTime.h:170:35: note: member function 'opentime::v1_0::RationalTime::operator-=' is declared const here
    constexpr RationalTime const& operator-=(RationalTime other) noexcept
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/nporcino/build/local/include/opentime/rationalTime.h:175:20: error: cannot assign to non-static data member within const member function 'operator-='
            _rate  = other._rate;
            ~~~~~  ^
/Users/nporcino/build/local/include/opentime/rationalTime.h:170:35: note: member function 'opentime::v1_0::RationalTime::operator-=' is declared const here
    constexpr RationalTime const& operator-=(RationalTime other) noexcept
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/nporcino/build/local/include/opentime/rationalTime.h:179:20: error: cannot assign to non-static data member within const member function 'operator-='
            _value -= other.value_rescaled_to(_rate);
            ~~~~~~ ^
/Users/nporcino/build/local/include/opentime/rationalTime.h:170:35: note: member function 'opentime::v1_0::RationalTime::operator-=' is declared const here
    constexpr RationalTime const& operator-=(RationalTime other) noexcept
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
6 errors generated.

[darbyjohnston]

Strange, I wonder how that got by the other compilers.

Is the inline keyword needed since the function definition is inside the class?

[meshula]

Older versions of xcode/clang don't flag it either, and by older, I mean from versions released earlier this year.

inline is intended as a hint that the code should be inlined, as opposed to having a function created and called. I think it's still the case that it behaves that way, even inside a class although I'll admit the C++ spec is getting too large to retain in my monkey brain. My thought was that it inline is still a hint to inline, then it's appropriate for these for speed.

[darbyjohnston]

I found this FAQ from a quick search that looks like the "inline" is not needed when the function is inside the class body:
"Is there another way to tell the compiler to make a member function inline?"
https://isocpp.org/wiki/faq/inline-functions#inline-member-fns-more

I liked this quote from the description:

This is often more convenient than the alternative of defining your inline functions outside the class body. However, although it is easier on the person who writes the class, it is harder on all the readers since it mixes what a class does (the external behavior) with how it does it (the implementation). Because of this mixture, you should define all your member functions outside the class body if your class is intended to be highly reused and your class’s documentation is the header file itself. This is another application of Spock’s logic: the needs of the many (all the people reusing your class) outweigh the needs of the few (those who maintain your class’s implementation) or the one (the class’s original author)

[meshula]

I looked up the spec ~ https://en.cppreference.com/w/cpp/language/inline ~

The inline specifier, when used in a function's decl-specifier-seq, declares the function to be an inline function.
A function defined entirely inside a class/struct/union definition, whether it's a member function or a non-member friend function, is implicitly an inline function [if it is attached to the global module (since C++20)].

Sounds like removing it is fine since inline is at best a hint anyway. However the meaning flip-flops under C++20, if we introduce an opentimelineio module???! Good grief!

We might be forced to move to the out-of-class definition pattern at that point, but I don't want to take on a refactoring of this header to get over this clang-bump.

I don't think Spock would approve of his name being used to justify C++ isms. I have it on good authority he programs with a version of [INCITS 226-1994 S20018]* updated to be compatible with Daystrom's latest positronic systems.

• http://www.lispworks.com/documentation/HyperSpec/Front/index.htm

[codecov-commenter]

Codecov Report

Merging #1242 (dbf6d67) into main (aa5351c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1242   +/-   ##
=======================================
  Coverage   86.15%   86.15%           
=======================================
  Files         196      196           
  Lines       19632    19632           
  Branches     2302     2302           
=======================================
  Hits        16913    16913           
  Misses       2161     2161           
  Partials      558      558           

Flag	Coverage Δ
py-unittests	86.15% <ø> (ø)

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

Impacted Files	Coverage Δ
src/opentime/rationalTime.h	90.00% <ø> (ø)

---

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 aa5351c...dbf6d67. Read the comment docs.

[meshula]

PS, per @darbyjohnston's note, I removed the inline keyword.