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.

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

First pass at adding constexpr where useful #21

Merged
merged 1 commit into from
Jul 15, 2020
Merged

First pass at adding constexpr where useful #21

merged 1 commit into from
Jul 15, 2020

Conversation

oxt3479
Copy link
Contributor

@oxt3479 oxt3479 commented Jul 3, 2020

This is a first pass through Imath to add constexpr to functions that are contenders for compiler optimization.

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
Copy link
Contributor

@lgritz lgritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's here is certainly LGTM and acceptable (and very welcome!), and as far as I'm concerned, we should merge it even if we add more constexpr later.

But what I can't tell from these diffs is the degree to which this job is complete.

Are there methods of these classes that you were not able to constexpr? Can you enumerate or summarize which those were?

Also note that the rules for what can be constexpr changed/expanded a bit from C++11 to 14 to 17. I assume that this PR is for the "least common denominator" -- using C++11 rules. If there are yet other things that can't be constexpr under C++11 but can be for 14 or 17, we should at least note them in comments, add a "long-term issue" to remind us to revisit once our supported minimum is bumped to C++14, or consider using a macro such as these that I've found helpful: https://github.com/OpenImageIO/oiio/blob/master/src/include/OpenImageIO/platform.h#L77

@oxt3479
Copy link
Contributor Author

oxt3479 commented Jul 15, 2020

Concerning completeness: Every file within the Imath/Imath subdirectory has constexpr added wherever possible in accordance with the c++14 standard. The is except for methods which return void, as these will only be 'fruitful' when they return an error which did not seem worth any added compile time.

Discretion regarding c++11 was overlooked but is being addressed in a separate pull request using a macro to differentiate.

@oxt3479 oxt3479 merged commit f0da60d into AcademySoftwareFoundation:master Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants