xxx.h, xxx.{cc,cu}, xxx_test.{cc,cu} in the same directory #2251
Conversation
| cc_library(place SRCS place.cc) | ||
| cc_library(ddim SRCS ddim.cc) | ||
| cc_library(place SRCS place/place.cc) | ||
| cc_library(ddim SRCS ddim/ddim.cc) | ||
|
|
||
| if(WITH_TESTING) |
There was a problem hiding this comment.
@gangliao WITH_TESTING 和 WITH_GPU等判断是不是可以放在cc_test和nv_test的定义里面，这样CMakeLists.txt看起来会更简洁一些。
| cc_library(place SRCS place.cc) | ||
| cc_library(ddim SRCS ddim.cc) | ||
| cc_library(place SRCS place/place.cc) | ||
| cc_library(ddim SRCS ddim/ddim.cc) | ||
|
|
||
| if(WITH_TESTING) |
| @@ -1,4 +1,4 @@ | |||
| #include "paddle/majel/ddim.h" | |||
| #include "paddle/majel/ddim/ddim.h" | |||
There was a problem hiding this comment.
I tend to think that we don't need to move each C++ module into a directory. In C++, a module is defined by a .cc file, optionally with interface declaration in a .h file. We can describe a C++ module by a CMake rule, e.g., cc_library(ddim SRCS ddim.cc). So it seems overkilling to move each C++ module into a directory.
There was a problem hiding this comment.
I agree that current CMake rule in Paddle majel with cc_library() and cc_test() has made module describing quite clearly, therefore simple module like ddim and place is not necessery to moved into a directory. However, if a module is really complicated, we should still gather related files up in a subdirectory.
This is my personal understanding, I'm not sure whether it is right ...
There was a problem hiding this comment.
I think you are right for the complicated case, e.g., paddle/parameter. For this case, let's move ddim back to paddle/majel.
There was a problem hiding this comment.
Got it. I'm going to close this PR.
fix #2209