-
Notifications
You must be signed in to change notification settings - Fork 286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C++ headers with separate declarations/definitions #1500
C++ headers with separate declarations/definitions #1500
Conversation
After the TSC meeting discussion, next steps:
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. |
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. |
I would like to work on those, but I didn't want to stack additional changes on top of the C++17 PR.
Can you say why not? |
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
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 my next observation is that the moved functions are trivial for the most part
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
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 proposalSo 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
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. |
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? |
Sure! BTW I'm perfectly fine with doxygenating just this one file to establish a pattern ~ I think that is what you are proposing. |
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