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

Fix for IDE headers #1279

Merged

Conversation

darbyjohnston
Copy link
Contributor

Hi, while I was working on the SSE PR I noticed that not all of the header files were showing up in the Visual Studio IDE. This PR adds the missing header files in the "OpenEXR" library to the CMakeLists.txt file so that they can be browsed in the IDE. I added these to the SOURCES variable assuming they are private and should not be installed.

I also noticed in the "Iex" library that IexMathFpu.h was missing from the CMakeLists.txt file. This actually looks like a public header so I added it to the HEADERS variable to be installed with the others.

Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
@meshula
Copy link
Contributor

meshula commented Sep 26, 2022

I have a memory that we didn't publish IexMathFpu.h because we don't want people to use OpenEXR to mess with the FP settings, and we were worried about outright dropping it in case someone complained. So I think we left it, unpublished, meaning to come back to delete it later after a reasonable period. I think it would be best if we don't start publishing it at this late date; probably time to delete it for real...

Other than that, exposing the headers for IDEs makes sense. With the way you've set it up does VS automatically sort the headers into a Headers or Includes folder? I usually set it up manually with a folder group, which is why Im asking (https://cmake.org/cmake/help/latest/command/source_group.html)

Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
@darbyjohnston
Copy link
Contributor Author

OK, thanks, I moved IexMathFpu.h to the SOURCES variable so it shows up in the IDE but does not get installed. Maybe it should be removed as a separate PR?

It was a bit confusing which headers are public and which are private, some of them have a comment they are private but most do not. Maybe a naming scheme would help? I noticed the "OpenEXRCore" library prefixes private headers with "internal_".

CMake checks the file extensions and puts them into the appropriate "Header Files" and "Source Files" folders in the VS Solution Explorer. Which makes sense because SOURCES and HEADERS are just arbitrary variables, all of the files get passed to the CMake "add_library" command.

@meshula
Copy link
Contributor

meshula commented Sep 26, 2022

It was a bit confusing which headers are public and which are private, some of them have a comment they are private but most do not. Maybe a naming scheme would help? I noticed the "OpenEXRCore" library prefixes private headers with "internal_".

another common pattern is to nest the headers in internal/ if they must be installed, but users shouldn't use them. I wonder if those private headers are in the "must install but not be used by users" category?

@darbyjohnston
Copy link
Contributor Author

They are added to the SOURCES variable so it looks like they are not installed.

@meshula
Copy link
Contributor

meshula commented Sep 27, 2022

Cool. A naming scheme would help IMO, following OpenEXRCore as you noted, makes sense.

@darbyjohnston
Copy link
Contributor Author

Is there anything else I need to do for this PR? As I mentioned I'm happy to create a separate PR for removing IexMathFpu.h if that's appropriate.

@meshula
Copy link
Contributor

meshula commented Oct 3, 2022

Won't affect any work in progress, so I'm going ahead with a merge

@meshula meshula merged commit df4f7c9 into AcademySoftwareFoundation:main Oct 3, 2022
@darbyjohnston
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants