#define duplication between CMakeLists and caffe_config.h

intelfx commented 7 years ago

(This is a copy of issue BVLC/caffe#4625, related to PR BVLC/caffe#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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Ok, this is resolved then.

intelfx commented 7 years ago

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

naibaf7 commented 7 years ago

@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 :)

naibaf7 / caffe

#define duplication between CMakeLists and caffe_config.h #41