Skip to content
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

truncation of struct initializers results in compiler warnings #248

Closed
diablodale opened this issue Nov 9, 2021 · 6 comments
Closed

truncation of struct initializers results in compiler warnings #248

diablodale opened this issue Nov 9, 2021 · 6 comments
Assignees

Comments

@diablodale
Copy link
Contributor

The codebase (including the precompiled Windows distribution headers) has initializers that are truncating their values. This results in compiler warnings. This and other such errors I've found #71 and #247 and #79 suggest the codebase has not been compiled with analyzers and warnings to surface easily found bugs.

Three incidents I found in my compile are double truncated to float. As they are initializers, I would hope (but it is not promised across all compilers) that the executable would only store a float in the static init data. But if the compiler does not, then the size is doubled (4 -> 8 bytes) and additional bytecode is needed to convert the double -> float at runtime.

Naturally, none of that is desired and the fix is easy. Put an f after the number literal.

// errant
float epsilon = 0.01;

// correct
float epsilon = 0.01f;

Setup

  • all compilers and OS
  • throughout codebase, yet can also be seen in the headers of precompiled Windows distrib: depthai-core-v2.11.1-win64

Repro

  1. Enable warnings and errors in compiler. e.g. in MSVC it is with /W4
  2. Compile an app that uses the depthai-core-v2.11.1-win64 and simple code that cascades to headers like RawFeatureTrackerConfig.hpp. Most any code, even just a dai device, will do this.

Result

...
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/datatype/RawFeatureTrackerConfig.hpp(99): warning C4305: 'initializing': truncation from 'double' to 'float'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/datatype/RawFeatureTrackerConfig.hpp(105): warning C4305: 'initializing': truncation from 'double' to 'float'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/datatype/RawFeatureTrackerConfig.hpp(174): warning C4305: 'initializing': truncation from 'double' to 'float'
...

Expected

No warnings or errors. No increase in exe size, no runtime convert double->float.

@Luxonis-Brandon
Copy link
Contributor

Thanks for the thorough report. Brought up internally. :-)

@SzabolcsGergely SzabolcsGergely self-assigned this Nov 9, 2021
@diablodale
Copy link
Contributor Author

Also found truncation in depthai SDK itself (I'm getting closer to a full depthai sdk compile)
Unclear if hard truncs on floats indicating dimensions is ok, or needs to round up, down, towards zero, accounting style, etc.

image

@diablodale
Copy link
Contributor Author

@szabi-luxonis I have a PR for some of these if you don't yet have the work started. It needs changes in this repo and changes in depthai-shared.

@SzabolcsGergely
Copy link
Collaborator

@szabi-luxonis I have a PR for some of these if you don't yet have the work started. It needs changes in this repo and changes in depthai-shared.

Please do.
Once PR is in place I will update the firmware. (core and firmware must point to the same depthai-shared commit).

@diablodale
Copy link
Contributor Author

@szabi-luxonis I have a PR ready. What is your team's standard approach for contributors wrt which branch/commit on which PRs should be?
For example, my changes to depthai-shared are currently branched from 99f9c588547e828663bb42e89f816d184d8dcff5 which is the submodule commit for v2.11.1.
Or should I rebase it on to the most recently commit of main and submit the PR that way?

@SzabolcsGergely
Copy link
Collaborator

@szabi-luxonis I have a PR ready. What is your team's standard approach for contributors wrt which branch/commit on which PRs should be? For example, my changes to depthai-shared are currently branched from 99f9c588547e828663bb42e89f816d184d8dcff5 which is the submodule commit for v2.11.1. Or should I rebase it on to the most recently commit of main and submit the PR that way?

Please target latest develop with PR.

diablodale added a commit to diablodale/depthai-shared that referenced this issue Nov 16, 2021
diablodale added a commit to diablodale/depthai-core that referenced this issue Nov 16, 2021
diablodale added a commit to diablodale/depthai-core that referenced this issue Nov 17, 2021
diablodale added a commit to diablodale/depthai-core that referenced this issue Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants