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

FastLED compiler warnings on gcc/linux #985

Open
marcmerlin opened this issue Mar 19, 2020 · 7 comments
Open

FastLED compiler warnings on gcc/linux #985

marcmerlin opened this issue Mar 19, 2020 · 7 comments

Comments

@marcmerlin
Copy link
Contributor

@marcmerlin marcmerlin commented Mar 19, 2020

I'm not sure if they are worth fixing/easy to fix. Full list attached:
out.txt

libraries/FastLED/colorutils.h: In copy constructor ‘CHSVPalette16::CHSVPalette16(const CHSVPalette16&)’:
libraries/FastLED/colorutils.h:455:69: warning: ‘void* memmove(void*, const void*, size_t)’ writing to an object of type ‘struct CHSV’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]
         memmove8( &(entries[0]), &(rhs.entries[0]), sizeof( entries));
                                                                     ^
In file included from libraries/FastLED/controller.h:9,
                 from libraries/FastLED/FastLED.h:47,
                 from examples/GFX_Table_Mark_Estes/neomatrix_config.h:112,
                 from examples/GFX_Table_Mark_Estes/Table_Mark_Estes.ino:5,
                 from src/main.cpp:13:
libraries/FastLED/pixeltypes.h:23:8: note: ‘struct CHSV’ declared here
 struct CHSV {
        ^~~~
In file included from libraries/FastLED/FastLED.h:58,
                 from examples/GFX_Table_Mark_Estes/neomatrix_config.h:112,
                 from examples/GFX_Table_Mark_Estes/Table_Mark_Estes.ino:5,
                 from src/main.cpp:13:
libraries/FastLED/colorutils.h: In member function ‘CHSVPalette16& CHSVPalette16::operator=(const CHSVPalette16&)’:
libraries/FastLED/colorutils.h:459:69: warning: ‘void* memmove(void*, const void*, size_t)’ writing to an object of type ‘struct CHSV’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]
         memmove8( &(entries[0]), &(rhs.entries[0]), sizeof( entries));
                                                                     ^
In file included from libraries/FastLED/controller.h:9,
                 from libraries/FastLED/FastLED.h:47,
                 from examples/GFX_Table_Mark_Estes/neomatrix_config.h:112,
                 from examples/GFX_Table_Mark_Estes/Table_Mark_Estes.ino:5,
                 from src/main.cpp:13:
libraries/FastLED/pixeltypes.h:23:8: note: ‘struct CHSV’ declared here
 struct CHSV {
        ^~~~
In file included from libraries/FastLED/FastLED.h:58,
                 from examples/GFX_Table_Mark_Estes/neomatrix_config.h:112,
                 from examples/GFX_Table_Mark_Estes/Table_Mark_Estes.ino:5,
                 from src/main.cpp:13:
libraries/FastLED/colorutils.h: In copy constructor ‘CHSVPalette256::CHSVPalette256(const CHSVPalette256&)’:
libraries/FastLED/colorutils.h:558:69: warning: ‘void* memmove(void*, const void*, size_t)’ writing to an object of type ‘struct CHSV’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]
         memmove8( &(entries[0]), &(rhs.entries[0]), sizeof( entries));
@5chmidti

This comment has been minimized.

Copy link

@5chmidti 5chmidti commented Mar 21, 2020

How about casting to a void* to get around it being recognized as a struct CHSV in memmove8. Because the copy constructor of CHSVPalette16 is using memmove anyways, there is no need to have it complain about not having a trivial copy constructor. (I guess this is true for all CHSVPalettes)
Would need to cast at ln 455 and 459 to test this. I cant, as I would need a newer gcc version

EDIT: see #971 (comment)

marcmerlin added a commit to MarcFork/FastLED that referenced this issue Mar 24, 2020
@marcmerlin

This comment has been minimized.

Copy link
Contributor Author

@marcmerlin marcmerlin commented Mar 24, 2020

Thanks @5chmidti this did quiet the warnings, I just wanted to make sure that this was ok/safe to do.
MarcFork@be2013e

@marcmerlin

This comment has been minimized.

Copy link
Contributor Author

@marcmerlin marcmerlin commented Mar 24, 2020

I cherry picked this into master: #986
Not sure who has review power outside of @kriegsman , do you by any chance?

@5chmidti

This comment has been minimized.

Copy link

@5chmidti 5chmidti commented Mar 25, 2020

I believe everybody can review changes in a pull request. Merging is reserved for people with write permission.

I looked at it once more and wrote something in the pr to completely remove the warning instead of going around it.

@5chmidti

This comment has been minimized.

Copy link

@5chmidti 5chmidti commented Mar 27, 2020

As I've said in the pr (should have stayed here, to keep the discussion in here):

I've done a quick test on the following (only if it compiles on c++14, but this should be correct according to the standard):
Why not just outright remove the warnings about CRGB and CHSV not being trivially copyable instead of casting the warning away?
By simply removing the user defined copy constructor' and copy assignments', CRGB and CHSV could simply be made trivially copyable by using the default copy constructor and copy assignment. One could write explicitly that we want to use the default with for example:
inline CRGB &operator=(const CRGB &rhs) __attribute__((always_inline)) = default;
Which I guess could be preferrable or even necessary because we have other copy assignments with other arguments for CRGB, as I don't know if the copy assignment for CRGB would be deleted.
After that the warnings shouldn't be there anymore, as the types would be trivially copyable.

I've gone ahead and commited these changes to my fork.

Performance wise there shouldn't be a difference between this and the master, but if someone knows how to reliably check, please do so. I've done some testing myself and found that they are about the same (no major difference / within margin of error).

@5chmidti

This comment has been minimized.

Copy link

@5chmidti 5chmidti commented Mar 28, 2020

I've opened #990
There was actually a similar discussion at #971

@marcmerlin

This comment has been minimized.

Copy link
Contributor Author

@marcmerlin marcmerlin commented Mar 28, 2020

Thanks for following up on this (been busy/distracted with other things)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.