-
Notifications
You must be signed in to change notification settings - Fork 5
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
Improve cmake and directory organization. #99
Conversation
Add braces to high level include files.
8becc67
to
a62c5ac
Compare
CMakeLists.txt
Outdated
"${PROJECT_SOURCE_DIR}/third_party/cereal/include" | ||
"${PROJECT_SOURCE_DIR}/third_party/fast-cpp-csv-parser/" | ||
) | ||
|
||
|
||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Werror -fexceptions -frtti") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
want to fix that too? take a look at what I did in orion...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just not pushed...?
@@ -13,6 +13,10 @@ | |||
#ifndef ALBATROSS_RANSAC_H | |||
#define ALBATROSS_RANSAC_H | |||
|
|||
#include "models/ransac.h" | |||
#include "Core" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... interesting. Totally legit, but I always like to make things consistent; looks like we've done <albatross/Core> in other places, like Tune.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which is better. If it's a relative path should it be "Core"
? or <albatross/Core>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Core", since it actually is local. Unless you think these files might move into different directories.
Also, not seeing where the dependency on the math library is captured. How are the examples linking? Do those specific examples just happen to not use the math library? Trying pulling "m" out of the unit test link libraries and see what happens... |
bedde20
to
4b97834
Compare
CMakeLists.txt
Outdated
add_subdirectory(tests) | ||
add_subdirectory(examples) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would set the compile flags above the add_subdirectory(*) and the move the actually setting of the compile options to the CMake where those targets are created because what could (is going to) happen eventually is someone is going to add a target in one of those subdirectories an not realize they also need to add it to albatross_BINARIES.
message(STATUS "Found: ${CLANG_FORMAT_PATH}") | ||
endif() | ||
find_program(CLANG_FORMAT NAMES | ||
clang-format-3.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? I thought we went 4.0 and 6.0? I can't even get 3.8 on 18.04 without sacrificing chickens...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was originally 3.8 and below, then at the beginning of this PR it was switched and I just reverted back to 3.8 since I can't get 4.0 to install on travis. #100
examples/sinc_example_utils.h
Outdated
@@ -13,8 +13,8 @@ | |||
#ifndef ALBATROSS_SINC_EXAMPLE_UTILS_H | |||
#define ALBATROSS_SINC_EXAMPLE_UTILS_H | |||
|
|||
#include "GP" | |||
#include "example_utils.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ordering?
tests/CMakeLists.txt
Outdated
COMMENT "Running unit tests" | ||
) | ||
|
||
EXCLUDE_FROM_ALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to put this on the previous line; that way it's not mingled with the source files.
on the whole, looking pretty good! |
4100643
to
f99223f
Compare
…k_origins ESD-2197: Enable tracking origins in memcheck Triggered-By: cmake dd6d9ede01cf4c9bc796e6256b40f17d3c139052
…k_origins (#304) ESD-2197: Enable tracking origins in memcheck Triggered-By: cmake dd6d9ede01cf4c9bc796e6256b40f17d3c139052
Move all the code to an
include
directory to allow for more conventional use.Thanks @martin-swift for the cmake makeover!