-
Notifications
You must be signed in to change notification settings - Fork 768
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
Header discipline in base #1108
Conversation
gtsam/base/Matrix.cpp
Outdated
@@ -33,6 +34,7 @@ | |||
#include <fstream> | |||
#include <limits> | |||
#include <iostream> | |||
#include <iostream> |
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.
duplicate
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.
If you reviewed everything and you trust me to fix the above, approve??
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.
There is this cool tool called include-what-you-use which we should probably leverage.
#include <iostream> | ||
#include <cstdlib> | ||
#include <fstream> | ||
#include <sstream> |
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.
Pretty sure fstream includes iostream for us, but I like being explicit so yay!
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.
Took a deeper look and LGTM
Yeah, tried that at one point. Its not so great for large repos with boost and Eigen. Maybe we should try it again. |
Trying to cut down on compile times, so