Feature/cpp overhaul - #7
magicbycalvin wants to merge 3 commits into
Conversation
…tructors for defining a polynomial with t0 and tf.
…they aren't provided ahead of time.
| LANGUAGES CXX | ||
| DESCRIPTION "BeBOT Library.") | ||
|
|
||
| add_subdirectory(src) |
There was a problem hiding this comment.
I wouldn't add_subdirectory here. I would expect you to really only have one or two build targets, a library to link to and maybe an executable, so it's better to just add_library(bebot src/...) here, etc rather than make a mostly redundant CMakeList in src to split across.
| set_tf(tf); | ||
| } | ||
|
|
||
| void BeBOT::set_cpts(const Eigen::Ref<Eigen::MatrixXd>& cpts) { |
There was a problem hiding this comment.
More descriptive naming is good. set_control_points isn't that many more characters. In general, we try to avoid writing C++ like it was C, and the horrible idmtcNmng_cnvns that exist in C shouldnt happen in C++
| @@ -0,0 +1,47 @@ | |||
| #include <bebot/bebot.hpp> | |||
| #include <Eigen/Dense> | |||
There was a problem hiding this comment.
Eigen is included in the header already
| @@ -0,0 +1,47 @@ | |||
| #include <bebot/bebot.hpp> | |||
| #include <Eigen/Dense> | |||
| #include <iostream> | |||
There was a problem hiding this comment.
no IO calls in this file, and there probably shouldn't be direct IO calls. Logging functions if necessary.
|
|
||
| BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts); | ||
| BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts, const double tf); | ||
| BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts, const double t0, const double tf); |
There was a problem hiding this comment.
I don't understand the purpose of multiple constructors. What happens if initial and final time are not specified? Scaled to 0..1? In that case, give the parameters default constructors.
| # and oldest tested versions of CMake. This will ensure | ||
| # you pick up the best policies. | ||
| cmake_minimum_required(VERSION 3.0.0) | ||
|
|
There was a problem hiding this comment.
You probably want to specify C++ language standard and compiler flags
| Eigen::MatrixXd _cpts; | ||
|
|
||
| public: | ||
| BeBOT() = default; |
There was a problem hiding this comment.
Default constructor doesn't instantiate cpts, t0 and tf -> undefined behaviour bonanza. Make one constructor BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts, const double t0=0, const double tf=1);
| #include <Eigen/Dense> | ||
|
|
||
| namespace bebot { | ||
| class BeBOT { |
There was a problem hiding this comment.
BeBOT is the name of the library but you probably don't want to name this class that. Maybe something like bebot::ControlProblem (this class describes a control problem to solve) or bebot::TrajectorySolver (this class solves for your trajectory), etc
|
|
||
| # # Link each target with other targets or add options, etc. | ||
|
|
||
| # #include_directories("${CMAKE_CURRENT_SOURCE_DIR}/include") |
There was a problem hiding this comment.
Oh yeah, just uncomment this and get rid of bebot subdirectory within include
| {3, 4, 6, 4, 7, 9} | ||
| }; | ||
|
|
||
| bebot::BeBOT c(cpts); |
There was a problem hiding this comment.
Where possible, prefer {} initialization. It's safer and avoids the most vexing parse.
2 participants
No description provided.