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

constexpr is invalid on is2D() #4540

Merged
merged 2 commits into from
Feb 8, 2025
Merged

Conversation

scourge411
Copy link
Contributor

The WLED_DISABLE_2D build flag fails to compile, because the is2D() member of the Segment is not a literal type:

In file included from wled00/fcn_declare.h:222:0,
    from wled00/wled.h:189,
    from wled00/FX.cpp:13:
     wled00/FX.h:714:27: error: 
     enclosing class of constexpr non-static member function 'bool Segment::is2D() const' is not a literal type
             inline constexpr bool is2D() const                                            { return false; }
                                  ^
In file included from wled00/fcn_declare.h:222:0,
    from wled00/wled.h:189,
    from wled00/FX.cpp:13:
        wled00/FX.h:361:16: note: 'Segment' is not literal because:
                 typedef struct Segment {
                                ^
wled00/FX.h:361:16: note:   'Segment' has a non-trivial destructor

Removing the constexpr allows it to compile

@DedeHai
Copy link
Collaborator

DedeHai commented Feb 6, 2025

compiles fine for me. what version and what env?

@scourge411
Copy link
Contributor Author

Not sure what you need for env; let me know if you need more. Basically running vsode/platformIO on the current tip of main, only change is adding the -D WLED_DISABLE_2D flag to the esp32dev config and building:

vscode 1.97
platformio: 6.1.16

(3.11.4) scourge411@ubuntu:~/WLED (main)$ git branch
* main

(3.11.4) scourge411@ubuntu:~/WLED (main)$ git log | head -5
commit 8c717537c489aae54c0f03091efc44fd6db0965e
Author: Blaž Kristan <blaz@kristan-sp.si>
Date:   Thu Feb 6 15:12:04 2025 +0100

    Lambda XY()

(3.11.4) scourge411@ubuntu:~/WLED (main)$ git st
## main...origin/main
 M platformio.ini

(3.11.4) scourge411@ubuntu:~/WLED (main)$ git diff platformio.ini
diff --git a/platformio.ini b/platformio.ini
index e775c61f..0534ad03 100644
--- a/platformio.ini
+++ b/platformio.ini

@@ -436,7 +437,7 @@ platform = ${esp32.platform}
 platform_packages = ${esp32.platform_packages}
 build_unflags = ${common.build_unflags}
 build_flags = ${common.build_flags} ${esp32.build_flags} -D WLED_RELEASE_NAME=\"ESP32\" #-D WLED_DISABLE_BROWNOUT_DET
-  ${esp32.AR_build_flags}
+  ${esp32.AR_build_flags} -D WLED_DISABLE_2D
 lib_deps = ${esp32.lib_deps}
   ${esp32.AR_lib_deps}
 monitor_filters = esp32_exception_decoder

(3.11.4) scourge411@ubuntu:~/WLED (main)$ npm --loglevel error  --no-fund --no-audit install
added 238 packages in 3s

(3.11.4) scourge411@ubuntu:~/WLED (main)$ platformio run --environment esp32dev 
Processing esp32dev (board: esp32dev; platform: espressif32@3.5.0; framework: arduino)
-----------------------------------------------------------------------------------------------------------------
Verbose mode can be enabled via `-v, --verbose` option
*** use existing my_config.h ***
npm run build
<<platformio doing platformio things>>
Compiling .pio/build/esp32dev/src/FX_fcn.cpp.o
Compiling .pio/build/esp32dev/src/alexa.cpp.o
In file included from wled00/fcn_declare.h:222:0,
                 from wled00/wled.h:189,
                 from wled00/FX_2Dfcn.cpp:10:
wled00/FX.h:713:27: error: enclosing class of constexpr non-static member function 'bool Segment::is2D() const' is not a literal type
     inline constexpr bool is2D() const                                            { return false; }
                           ^
In file included from wled00/fcn_declare.h:222:0,
                 from wled00/wled.h:189,
                 from wled00/FX_2Dfcn.cpp:10:
wled00/FX.h:361:16: note: 'Segment' is not literal because:
 typedef struct Segment {
                ^
wled00/FX.h:361:16: note:   'Segment' has a non-trivial destructor
*** [.pio/build/esp32dev/src/FX_2Dfcn.cpp.o] Error 1
*** [.pio/build/esp32dev/src/alexa.cpp.o] Error 1
*** [.pio/build/esp32dev/src/FX.cpp.o] Error 1
*** [.pio/build/esp32dev/src/FX_fcn.cpp.o] Error 1
==== [FAILED] Took 11.84 seconds ====
Environment    Status    Duration
-------------  --------  ------------
esp32dev       FAILED    00:00:11.836

@DedeHai
Copy link
Collaborator

DedeHai commented Feb 7, 2025

thanks for the feedback. I can see the error but only on ESP32dev env, the _V4 version does work so it seems to be compiler(setting) related. Also other envs based on V4 work fine.
@willmmiles any idea what is going on and what fix should be applied?

@willmmiles
Copy link
Member

It is likely a C++ version thing -- IIRC, constexpr rules were substantially extended in C++14.

I'd probably recommend removing constexpr for now, at least until we can enforce a specific newer C++ standard. It won't make any difference to the resulting binary -- the function call should be inlined to a constant anyways. I'm not sure that constexpr is helpful here anyways, as I don't think we'll ever be calling this function in some other constexpr context.

wled00/FX.h Outdated
@@ -711,7 +711,7 @@ typedef struct Segment {
void wu_pixel(uint32_t x, uint32_t y, CRGB c);
inline void fill_solid(CRGB c) { fill(RGBW32(c.r,c.g,c.b,0)); }
#else
inline constexpr bool is2D() const { return false; }
inline bool is2D() const { return false; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix the alignment of indentation and I will merge this.

@DedeHai
Copy link
Collaborator

DedeHai commented Feb 7, 2025

@scourge411 can you also update to latest main to resolve conflict?

@scourge411
Copy link
Contributor Author

Should be good now.

@DedeHai
Copy link
Collaborator

DedeHai commented Feb 8, 2025

thanks. for future reference: don't force push on branches in a PR, it can mess up the review.

@DedeHai DedeHai merged commit 95a10c6 into wled:main Feb 8, 2025
20 checks passed
@scourge411 scourge411 deleted the fix_DISABLE_2D branch February 9, 2025 04:50
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