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

Fix or silence warnings #53

Open
psiha opened this issue Nov 15, 2022 · 16 comments
Open

Fix or silence warnings #53

psiha opened this issue Nov 15, 2022 · 16 comments
Labels
bug Something isn't working

Comments

@psiha
Copy link
Contributor

psiha commented Nov 15, 2022

currently we need to disable:
"-Wconversion"
"-Wdocumentation"
"-Wunused-variable"
before
#include <kwk/utility/container/shape.hpp>
#include <kwk/utility/linear_index.hpp>

@psiha psiha added the bug Something isn't working label Nov 15, 2022
@jfalcou
Copy link
Owner

jfalcou commented Nov 15, 2022

o_O -Wdocumentation ??? Is that a clang specific flag ?

@psiha
Copy link
Contributor Author

psiha commented Nov 15, 2022

yes clang can detect broken doxygen documentation

@jfalcou
Copy link
Owner

jfalcou commented Nov 16, 2022

Good ! I didn't know it did that.
We'll try to see why the two other are not picked up by Wall Wother

@psiha
Copy link
Contributor Author

psiha commented Nov 16, 2022

for unused-variable you need to build release/without asserts (as some variables you use only for sanity checks)

@jfalcou
Copy link
Owner

jfalcou commented Nov 16, 2022

Will add a CI recipe for at least one Release mode test then.

@psiha
Copy link
Contributor Author

psiha commented Nov 16, 2022

Will add a CI recipe for at least one Release mode test then.

you didn't have that already?? 🙈 🙈 :P

we'd prefer you test a full -Ofast -flto for the release build :)

@jfalcou
Copy link
Owner

jfalcou commented Nov 16, 2022

Will add a CI recipe for at least one Release mode test then.
you didn't have that already?? 🙈 🙈 :P

I ported the CI script from EVE without rechecking them :p

we'd prefer you test a full -Ofast -flto for the release build :)

Can do

@jfalcou
Copy link
Owner

jfalcou commented Nov 16, 2022

-Wunused and -Wdocumentation has been fixed

-Wconversion is another beast. It complains when passing int to [] that 1.f+i can't be assigned to float in tests.
It also makes all our design of size beign signed go to the drain cause we need to add tons of cast when interacting
with some std components.

Not sure how to handle those :/

@jfalcou
Copy link
Owner

jfalcou commented Nov 16, 2022

OK found the one culprit, next PR will fix those

@psiha
Copy link
Contributor Author

psiha commented Nov 16, 2022

-Wunused and -Wdocumentation has been fixed

-Wconversion is another beast. It complains when passing int to [] that 1.f+i can't be assigned to float in tests. It also makes all our design of size beign signed go to the drain cause we need to add tons of cast when interacting with some std components.

Not sure how to handle those :/

You chose a signed size_type for vectorization-performance issues, avoiding exactly these warning issues and/or something else?

@jfalcou
Copy link
Owner

jfalcou commented Nov 16, 2022

In fact the main problem was the way we were capturing fixed size. Went back to fixed size being unsigned when built from _c and silenced warnings in a few places with pragma. Other fixes were stupid mistake and adapting tests.

This have been merged.
Tell me if it is enough.

jfalcou added a commit that referenced this issue Nov 17, 2022
We now test AppleClang 14 on Mac OS X 12.
Due to iains/gcc-12-branch#6 (comment) , we test
g++-11 on Mac OS X 11 till the XCode CLT issue is fixed github's side
@jfalcou
Copy link
Owner

jfalcou commented Nov 21, 2022

Closingfor now as multiple runs show no extra warning on our side

@jfalcou jfalcou closed this as completed Nov 21, 2022
@psiha
Copy link
Contributor Author

psiha commented Nov 22, 2022

kiwaku/include/kwk/detail/sequence/prefilled.hpp:65:81: error: non-constant-expression cannot be narrowed from type 'unsigned long long' to 'unsigned long' in initializer list [-Wc++11-narrowing]

                                return std::array<std::size_t, static_size>{m...};

while building for emscripten (which is a 32bit platform so that could be difference)...

@jfalcou
Copy link
Owner

jfalcou commented Nov 23, 2022

Oh I forgot about emscriptem.
Will fix this missign static_cast<> I guess then add an emscriptem target for CI

@psiha
Copy link
Contributor Author

psiha commented Nov 25, 2022

Oh I forgot about emscriptem. Will fix this missign static_cast<> I guess then add an emscriptem target for CI

Perhaps rather do not use 64bit types unconditionally - probably a size_t-like type would do in most those situations...
Also, even using explicitly 32bit types (instead of size_t) can give smaller codegen (if there is no mixing with 64bit types) when you know that you don't need the range (e.g. for 'certainly tiny' numbers like ranks/numbers of dimensions)..

@jfalcou jfalcou reopened this Nov 25, 2022
@jfalcou
Copy link
Owner

jfalcou commented Nov 25, 2022

Perhaps rather do not use 64bit types unconditionally - probably a size_t-like type would do in most those situations... Also, even using explicitly 32bit types (instead of size_t) can give smaller codegen (if there is no mixing with 64bit types) when you know that you don't need the range (e.g. for 'certainly tiny' numbers like ranks/numbers of dimensions)..

std::size_t is supposed to be the size of the system memory. It is not 64 bits ona 32 bits system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants