-
Notifications
You must be signed in to change notification settings - Fork 16
Cleanup in common/include/utils.hpp
#1002
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
Conversation
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
9d7f327 to
fbe02d0
Compare
|
The test failure in the CI is actually really interesting. The cause is the commit replacing
https://godbolt.org/z/r1r6j3fP8 This breaks code which was implicitly relying on |
|
Ok, the problem is that the What would be the correct way to fix this? We don't have any metadata about how many samples may be buffered during processing. I see two possible fixes here:
|
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
stv0g
left a comment
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.
Great work! This is really a good example of how a PR should be structured and presented!
A pleasure to review :)
I've gone through our
utils.hppheader and removed macros and functions that map directly to the standard library.Each change is a separate commit and can be dropped if deemed undesirable.
Almost all changes could be done using simple automation like e.g. replacing
ARRAY_LENwithstd::size.The standard library alternatives often provide stronger type safety and prevent subtle bugs.
ARRAY_LENon a decayed pointer to an array would just return 1, whilestd::sizeonly works for non-decayed arrays or library containers.std::swapwill respect ADLswapspecializations which may elide unneccessary copies.std::minandstd::maxdon't compile if the participating integer types differ. This forces us to highlight narrowing integer conversions with an explicitstatic_cast, instead of an implicit truncation. (We are often mixingsize_tandunsigned intlengths)<bit>header which has functions replacing macros likeIS_POW2/LOG2_CEIL