-
Notifications
You must be signed in to change notification settings - Fork 41
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
Refactoring code and fixes: cmake, crashes and codecy #1005
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very easy to review! It's clear that the second commit changes nothing, which let me focus on the rest of the code. I have two minor comments on the last commit, I leave them to you (i.e. do as you prefer).
Apparently we see in #999 that adding the header files to CMakeLists
isn't necessary for VS 2022. I think we can still merge this PR while we figure out whether this is indeed the case, since the change could be reverted without breaking the blame too much (I would prefer to have only the .cpp
if possible, since that's how CMake is intended to work and it's less verbose).
When I run a build of this PR I am seeing a lot more warnings that from what we see on master branch right now.
Not sure if this is an issue or not, just making mention of it. |
Thank you for writing about this. |
The headers were added because Microsoft Visual Studio's "Solution" view doesn't show the headers. However, VS' built-in CMake handling doesn't use the solution view and is based on folders (much like CMake itself). There is no recommendation from the CMake side on how to handle this problem with CMake's built-in generators for Visual Studio. Following the VS documentation and the instructions added in longturn#999, one can get (at least) decent VS support with IntelliSense working. This doesn't require keeping the headers in the CMakeLists, so remove them for more idiomatic CMake code. This reverts part of commit 6e38a15. See longturn#1005.
The headers were added because Microsoft Visual Studio's "Solution" view doesn't show the headers. However, VS' built-in CMake handling doesn't use the solution view and is based on folders (much like CMake itself). There is no recommendation from the CMake side on how to handle this problem with CMake's built-in generators for Visual Studio. Following the VS documentation and the instructions added in #999, one can get (at least) decent VS support with IntelliSense working. This doesn't require keeping the headers in the CMakeLists, so remove them for more idiomatic CMake code. This reverts part of commit 6e38a15. See #1005.
No description provided.