-
Notifications
You must be signed in to change notification settings - Fork 166
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
Fix some compiler warnings #207
base: main
Are you sure you want to change the base?
Conversation
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.
Fixing compiler warnings is always a good deed! :)
@@ -854,7 +854,7 @@ auto repr(const Real<N, T>& x) | |||
{ | |||
std::stringstream ss; | |||
ss << "autodiff.real("; | |||
for(auto i = 0; i <= N; ++i) | |||
for(decltype(N) i = 0; i <= N; ++i) |
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 are other "styles" possible here, e.g.
for(auto i = 0u; ...
for(std::size_t i = 0; ...
Which is the preferred style for autodiff?
assert(arg.size() == dir.size()); | ||
for(auto i = 0; i < dir.size(); ++i) | ||
size_t len = dir.size(); | ||
for(decltype(len) i = 0; i < len; ++i) |
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.
One could remove len
if we would use auto i = 0u
or something similar.
Starting from cmake 3.24 there is also a target property for it: COMPILE_WARNING_AS_ERROR. |
This PR corrects some compiler warnings:
ForEachWrtVar
inforward/utils/gradient.hpp
-Wsign-compare
, stemming from a mismatch in types between the loop index and a some size variable in several places.After this change, I can compile cleanly with
-Wall -Werror
.What are your thoughts on adding
-DCMAKE_CXX_FLAGS="-Wall -Werror"
to the cmake configure step in CI? It makes it nicer to include a header a only library in other projects if it will compile without warnings, and this would help guarantee that.