-
Notifications
You must be signed in to change notification settings - Fork 120
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
correct float literals, 32/64 trunc, unref vars #268
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.
CI is failing due to style checks.
clangformat
target should solve it, but I assume you don't have it installed on windows?
It is enabled in cmake, but such isn't available in typical Windows installations so cmake it likely quietly ignoring that target. Alternatively, I used the VSCode c++ extension from Microsoft and have enabled. It does apply clang-format when I type a semicolon. But in many of these, I never typed a semi...I just inserted the casts inside the line. So it never formatted. I've turned on the additional option to clang-format on save of the file. Makes me a bit paranoid because I'll likely not notice the reformatting, but I get your team's need for this and I can work with that. That all ok with u? |
Yes, that's fine. |
🤔 the CI here isn't getting the forcepush I did. Its available at https://github.com/diablodale/depthai-core/tree/fix248-trunc-2 but it isn't showing up on the tabs here (and therefore not reprocessing it) |
Reran the checks - will merge afterwards 👍 |
@@ -136,8 +136,8 @@ bool matInv(std::vector<std::vector<float>>& A, std::vector<std::vector<float>>& | |||
|
|||
std::vector<float> temp; | |||
// Find Inverse using formula "inverse(A) = adj(A)/det(A)" | |||
for(size_t i = 0; i < A.size(); i++) { | |||
for(size_t j = 0; j < A.size(); j++) { | |||
for(size_t i = 0; i < A.size(); ++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.
Just curious on the i++
-> ++i
changes, were there any warnings triggered by the previous version, or it's more like a style preference?
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.
Porbably style, pre-increment is faster than post-increment, theoretically, but not for POD.
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.
pre is my preferred for best opportunity for optimization, see notes https://en.cppreference.com/w/cpp/language/operator_incdec
No guarantee compilers old/new on all platforms won't use the slow path on POD. So I prefer pre.
I did not do it everywhere i that matrix code as some places need the post behavior.
FYI, the Eigen library is fantastically excellent and all header. Even for light use like cali processing. They've been testing and optimizing across years and more platforms than most anyone could redo.
The depthai-core portion of fixes for #248
Together with the already merged depthai-shared portion, they eliminate hundreds of warnings to lessen noise.