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

#define duplication between CMakeLists and caffe_config.h #41

Closed
intelfx opened this issue Aug 24, 2016 · 5 comments
Closed

#define duplication between CMakeLists and caffe_config.h #41

intelfx opened this issue Aug 24, 2016 · 5 comments

Comments

@intelfx
Copy link

intelfx commented Aug 24, 2016

(This is a copy of issue BVLC#4625, related to PR BVLC#4609.)

While refactoring the buildsystem I've noticed that several USE_xxx definitions are duplicated in caffe_config.h (via #cmakedefine) and in CMake code (via add_definitions() or equivalent).

Because I'm going to post the equivalent PR here due to complex merge conflicts, I'd better ask you the same question. What is the reason for this? Is it on purpose? Originally, I proposed to get rid of #cmakedefines in caffe_config.h.in and instead add these definitions from CMakeLists because this way the definitions can be properly exported to the library clients, but I see that in this fork you actually did the inverse — i. e. removed more defines from CMakeLists and moved them to caffe_config.h. So, there are two ways:

  • to remove all add_definitions() from CMakeLists, move them as #cmakedefines to caffe_config.h.in, install that file and include it from all public headers (probably splitting caffe_config.h.in in two files for private and public definitions);
  • to remove all #cmakedefines from caffe_config.h.in and instead use code in CMakeLists to add them.

The first option can be deemed more "stylish" because the definitions are then more easily visible and they are automatically generated from CMake variables. The second option is easier to implement because there is no need to modify C code and inspect all headers.

@naibaf7
Copy link
Owner

naibaf7 commented Aug 24, 2016

Yes that makes sense, the CMake system needs a makeover.
I think back then I went with was easier for me to make compatible alongside the GNU Makefile system, but I understand your points...
I'll think about it :)

@intelfx
Copy link
Author

intelfx commented Aug 25, 2016

So, I tentatively took the path of replacing #cmakedefines with add_definition() calls and finished the refactoring, just for you to see how it looks (and because I need this branch on Android right now, even if it will be re-done in future).

@naibaf7
Copy link
Owner

naibaf7 commented Aug 25, 2016

Ok, this is resolved then.

@naibaf7 naibaf7 closed this as completed Aug 25, 2016
@intelfx
Copy link
Author

intelfx commented Aug 25, 2016

Thanks. If you ultimately decide that #cmakedefine was the way to go, drop me a note.

@naibaf7
Copy link
Owner

naibaf7 commented Aug 25, 2016

@intelfx
I'm currently still using the GNU Makefiles for my own experiments, so as long as nobody complains and it works, it's good as it is :)

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

No branches or pull requests

2 participants