Skip to content

Conversation

@Altren
Copy link
Contributor

@Altren Altren commented Sep 29, 2024

Initially I wanted to go deeper with moving more stuff into cpp files where possible, but ClangBuildAnalyzer did not revealed any obvious parts, so I decided to commit only preparation changes.
Any suggestions what can be moved into cpp are welcome, and I'll create another PR

@liuzicheng1987
Copy link
Contributor

Hi @Altren , why do you want to split the .cpp file into three smaller files?

The reason I specifically didn't do this is that I want to make installation as simple as possible. Some people don't use CMake packages, but instead they want to simply include the code in their own build process.

Being able to simply copy a single source file into your own source code makes this very easy.

@liuzicheng1987
Copy link
Contributor

The changes to the CMake file seem fine, but I haven't taken a closer look yet...reducing duplicate code is always a good idea...

@Altren
Copy link
Contributor Author

Altren commented Sep 30, 2024

I splitted this into files, because I wanted to move more code into .cpp files and adding it into single file doesn't looks ok for me.

Some people don't use CMake packages, but instead they want to simply include the code in their own build process.
Being able to simply copy a single source file into your own source code makes this very easy.

Well, they already copy multiple headers anyway. I you want to keep it simple for such use cases I can add reflectcpp.cpp back and #include all new .cpp files into it.

@liuzicheng1987
Copy link
Contributor

@Altren , at the moment reflectcpp.cpp is fairly short...I don't think there is much of a need to split it up further. It's just going to increase compile time and make things unnecessarily complicated.

However, if you can find significant portions of the code that you can put into source files, we could seriously think about splitting it up. I would be very happy if that were possible.

@Altren
Copy link
Contributor Author

Altren commented Oct 2, 2024

I updated my changes, so that both problem you mentioned are solved:
Compile time is the same as it was, since reflectcpp.cpp is a simple inclusion of other source files.
Only reflectcpp.cpp need to be added to the build, like it was before

@Altren
Copy link
Contributor Author

Altren commented Oct 2, 2024

But I'm ok if we keep this not merged until I'll do more useful changes, that require this split
And sorry for the force pushes, I used wrong commit author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants