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

Final correction gcc 10 compile error #199 with sharebrained feedback #419

Conversation

Brumi-2021
Copy link
Contributor

This PULL request is INTEGRATING yesterday PR ("New feature gcc 10 compile errors #199" )
and in fact, it has the final refine correction . (NO NEED TO APPLY BOTH PR'S , just that one is enough because it is integrating both commits ) .

image

So we need to merge to the next , just that one PR, , and ignore the previous one . , for solving the GCC 10 compile error due to syntax problem)
already pointed in previous discussion , #199

I will not repeat all yesterday comments inside my previous PR. , that are correct, but the solution was not complelety OK.
After commenting the previous PR with sharebrained (original owner of those tow files, and many others) , he pointed me that

(sharebrained comments ) ----------------------------------------------
" it looks like the proposed code changes will change what the assertion is testing. Before, it was making sure that the number of elements in the array was a power of 2. With the change, it is making sure the number of bytes of the array is a power of two. Those are different things, and changes what the assertion is testing.
I was not familiar with the GCC10 issue.

It does appear to be my code, yes. I would recommend removing the static_assert, as the C++ standard apparently does not permit that test to be evaluated at compile time. "--------------

And sharebrained , was completelly correct, . But I took his feedback to not remove that assert, and apply the small corrections, to keep that compile safe protection , as it was originally intended.

In my previous PR, I was confusing array size (in terms of bytes) , with number of array elements (not bytes)
When applying in previous PR (NOT CORRECT ) :

static_assert(power_of_two(sizeof(s)), "Array size must be power of 2");
with , s = std::array<std::complex, 256> channel_spectrum { };

1-) Confirmed that previous PR , sizeof(s) = 2048 (not 256 array elements as it should be )
(256 elements * 2 float (real & imaginary part) * 4 bytes/ float = 2048 bytes.

Then sizeof(s) was NOT = 256 elements ,
but, as it was also a power of 2 ,( and it was working well, but it was not same assert as in the original code ) . Then NOT CORRECT solution.

2-) Now the ARRAY_ELEMENTS(s) = 256 elements ,
it does not matter how we define each element of the array , float , complex float , int, ….

3-) Tested , that when intentionally changed the definition to a NON POWER of 2 , example 255, it is also detected at compiling-time (as it should be )

Then we addressed it , with the following two changes , (ALREADY MODIFYIED from previous yesterday PR)

In this PR , we define a new auxiliar compile time operator , based on the previous sizeof() ,
#define ARRAY_ELEMENTS(x) (sizeof(x) / sizeof(x[0]))
/* sizeof() compile-time operator that returns #bytes of (data type). We used it to get #elements_array */

static_assert(power_of_two(ARRAY_ELEMENTS(s)), "Array number of elements must be power of 2"); // c/m compile error GCC10 , OK for all GCC versions.

Special thanks to sharebrained , for his time, and support , to review my previous PR proposal , and for his wise feedback ,
The previous PR, was working , avoiding GCC compile error, but not doing exactly what it should do and protect.
This one Yes ! We are respecting original safe protection , in different syntax to avoid GCC compile error..

… the size of the array ,but with slightly different sintax.
…rors_#199 (based on sharebrained indications)
@ArjanOnwezen
Copy link
Contributor

ArjanOnwezen commented Nov 7, 2021

@Brumi-2021 Looks good! Perhaps both commits can be squashed into one.

@eried eried merged commit dc06950 into portapack-mayhem:next Nov 24, 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

Successfully merging this pull request may close these issues.

3 participants