Skip to content
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.

Sign up for GitHub

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

Jump to bottom

C++ headers with separate declarations/definitions #996

Open
darbyjohnston opened this issue Jun 22, 2021 · 6 comments · May be fixed by #1500
Open

C++ headers with separate declarations/definitions #996

darbyjohnston opened this issue Jun 22, 2021 · 6 comments · May be fixed by #1500
Labels
maintenance General cleanup and good code hygiene
Projects
C++ API

Comments

@darbyjohnston
Copy link
Contributor

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.
@meshula
Copy link
Collaborator

meshula commented Jun 22, 2021

If you're going to the trouble of breaking them out like that, it's worth, for each one, asking if it really belongs in the corresponding cpp file ~ in other words, asking why was this inline in the first place?

@darbyjohnston
Copy link
Contributor Author

darbyjohnston commented Jun 22, 2021 •
edited

Definitely, though a lot of them are templates so they need to be in the header files. I think these functions probably do not need to be inlined:

  • RationalTime::RationalTime(double value, double rate)
  • void SerializableObject::Reader::debug_dict()
  • void SerializableObject::Reader::error(()
  • void SerializableObject::Reader::_Resolver::finalize(error_function_t error_function)
  • SerializableObject::Writer::Writer(class Encoder& encoder)

Should I move those into the .cpp file in the PR?

@meshula
Copy link
Collaborator

meshula commented Jun 23, 2021

I agree on all of them except the constructor, that's preferably inlined no need to take a possible cache line load on such a common op

@darbyjohnston
Copy link
Contributor Author

Ah yes, sorry; for some reason I was under the impression that constructors were not inlined. I'll move the other ones to the .cpp file.

@darbyjohnston
Copy link
Contributor Author

I checked in a change to remove these functions from the headers in the PR:

  • void SerializableObject::Reader::debug_dict()
  • void SerializableObject::Reader::error(()
  • void SerializableObject::Reader::_Resolver::finalize(error_function_t error_function)

@reinecke reinecke added the maintenance General cleanup and good code hygiene label Jun 24, 2021
@darbyjohnston darbyjohnston mentioned this issue Aug 6, 2021
Closed
@darbyjohnston
Copy link
Contributor Author

Re-opening this issue since I think it would be a nice quality of life improvement for C++ developers. Especially if we add API documentation to the C++ headers at some point, moving the inline functions outside of the class declaration will remove noise and help readability. Here's a link again to one of the OpenEXR headers that follows this pattern:
https://github.com/AcademySoftwareFoundation/openexr/blob/main/src/lib/OpenEXR/ImfChannelList.h
It really makes the API pleasant to read without the implementation details getting in the way.

Unlike my previous PR I would suggest that a new PR should only separate the inline function definitions and declarations to keep it manageable. Additional work like determining whether functions should be inline or not could be done with follow up PRs.

@darbyjohnston darbyjohnston reopened this Dec 13, 2022
@darbyjohnston darbyjohnston linked a pull request Dec 13, 2022 that will close this issue
Draft
@jminor jminor added this to To Do in C++ API via automation Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
maintenance General cleanup and good code hygiene
Projects
OTIO - C++ API
Status: To Do
C++ API
  
To Do
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

C++ headers with separate declarations/definitions darbyjohnston/OpenTimelineIO
3 participants
@meshula @reinecke @darbyjohnston