Skip to content

xxx.h, xxx.{cc,cu}, xxx_test.{cc,cu} in the same directory #2251

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.

Sign up for GitHub

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

Closed
JiayiFeng wants to merge 3 commits into from
Closed

xxx.h, xxx.{cc,cu}, xxx_test.{cc,cu} in the same directory #2251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95ff6e3
xxx.h, xxx.{cc,cu}, xxx_test.{cc,cu} in the same directory
JiayiFeng May 24, 2017
034e6c2
correct misspelling
JiayiFeng May 24, 2017
e12f92a
remove useless comments
JiayiFeng May 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions paddle/majel/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
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)
Copy link
Contributor

@Xreki Xreki May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gangliao WITH_TESTING 和 WITH_GPU等判断是不是可以放在cc_test和nv_test的定义里面，这样CMakeLists.txt看起来会更简洁一些。

Copy link
Collaborator

@wangkuiyi wangkuiyi May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a new issue #2259 to record @Xreki 's suggestion.

add_subdirectory(test)
cc_test(ddim_test SRCS ddim/ddim_test.cc DEPS ddim)
cc_test(place_test SRCS place/place_test.cc DEPS place)

if(WITH_GPU)
nv_test(cuda_test SRCS cuda_test/cuda_test.cu)
nv_test(dim_test SRCS ddim/dim_test.cu DEPS ddim)
endif()

endif()
0 paddle/majel/test/cuda_test.cu → paddle/majel/cuda_test/cuda_test.cu
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion paddle/majel/ddim.cc → paddle/majel/ddim/ddim.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "paddle/majel/ddim.h"
#include "paddle/majel/ddim/ddim.h"
Copy link
Collaborator

@wangkuiyi wangkuiyi May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@JiayiFeng JiayiFeng May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Collaborator

@wangkuiyi wangkuiyi May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right for the complicated case, e.g., paddle/parameter. For this case, let's move ddim back to paddle/majel.

Copy link
Collaborator Author

@JiayiFeng JiayiFeng May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'm going to close this PR.


namespace majel {

Expand Down
2 changes: 1 addition & 1 deletion paddle/majel/ddim.h → paddle/majel/ddim/ddim.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include <stdexcept>
#include <vector>

#include "paddle/majel/dim.h"
#include "paddle/majel/ddim/dim.h"

namespace majel {

Expand Down
4 changes: 1 addition & 3 deletions paddle/majel/test/ddim_test.cc → paddle/majel/ddim/ddim_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
//#include <stdexcept>
//#include <unittest/unittest.h>
#include <sstream>
#include <vector>

#include "gtest/gtest.h"
#include "paddle/majel/ddim.h"
#include "paddle/majel/ddim/ddim.h"

TEST(DDim, Equality) {
// construct a DDim from an initialization list
Expand Down
0 paddle/majel/dim.h → paddle/majel/ddim/dim.h
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion paddle/majel/test/dim_test.cu → paddle/majel/ddim/dim_test.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#include <thrust/device_vector.h>
#include <sstream>

#include "paddle/majel/dim.h"
#include "paddle/majel/ddim/dim.h"
#include "gtest/gtest.h"

__global__ void test(majel::Dim<2>* o) {
Expand Down
2 changes: 1 addition & 1 deletion paddle/majel/place.cc → paddle/majel/place/place.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "paddle/majel/place.h"
#include "paddle/majel/place/place.h"

namespace majel {

Expand Down
0 paddle/majel/place.h → paddle/majel/place/place.h
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion paddle/majel/test/place_test.cc → paddle/majel/place/place_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "paddle/majel/place.h"
#include "paddle/majel/place/place.h"
#include <sstream>
#include "gtest/gtest.h"
#include "paddle/utils/Logging.h"
Expand Down
12 changes: 0 additions & 12 deletions paddle/majel/test/CMakeLists.txt
View file
Open in desktop

This file was deleted.