philbowles / PangolinMQTT

PangolinMQTT - ArduinoIDE client library for ESP8266, ESP32 and STM32-NUCLEO
Other
71 stars 21 forks source link

ESP32 compile error (xtensa-esp32-elf-g++) - constructor initialization order #10

Closed leifclaesson closed 4 years ago

leifclaesson commented 4 years ago

Hi Phil!

Having a problem compiling for ESP32. I will admit that I do not fully understand this particular syntax, and this is despite being a C++ programmer for 21 years. C++ is not a small standard, and it's a moving target.

I'm getting the following error, in many different forms:

In file included from C:\projects-public\arduino\PangolinMQTT\src\PangolinMQTT.cpp:26:0: C:\projects-public\arduino\PangolinMQTT\src\Packet.h: In constructor 'UnsubscribePacket::UnsubscribePacket(const string&)': C:\projects-public\arduino\PangolinMQTT\src\Packet.h:122:30: error: 'UnsubscribePacket::_topic' will be initialized after [-Werror=reorder]

(emphasis mine)

Looking at the code, it's complaining about this:

class UnsubscribePacket: public Packet { std::string _topic; public: UnsubscribePacket(const std::string& topic): _topic(topic),Packet(UNSUBSCRIBE,0,true) { _id=++_nextId; _begin=[this]{ _stringblock(CSTR(_topic)); }; _build(); } };

Rewriting it as follows appears to clear the error:

class UnsubscribePacket: public Packet { std::string _topic; public: UnsubscribePacket(const std::string& topic): Packet(UNSUBSCRIBE,0,true) { _topic=topic; _id=++_nextId; _begin=[this]{ _stringblock(CSTR(_topic)); }; _build(); } };

So, my questions are:

I'm not used to compounding constructor syntax so the difference is not obvious to me. I've never quite been able to wrap my head around this syntax so in my own code the constructor is mostly empty, and then there's a Init method, which usually takes a pointer to a class containing all the parameters, the constructor of which sets the default values. That is how I've written C++ for 20 years without figuring compounding constructors out, if you're curious :). Let's call it being human.

philbowles commented 4 years ago

Are you using platformIO? If so, you need to re-read the sections in the documentation relating to the bugs in platformIO that cause this and other problems. It is not a compiler error, it is a warning of the type the can be safely ignored. For reasons best known to the PIO team, they force all such warnings to cause an error by inserting -Werror=reorder into the build.

It compiles cleanly and runs cirrectly (last time I checked!) in ArduinoIDE.

leifclaesson commented 4 years ago

Hi Phil!

I'm not using platformio, I'm using Sloeber with the ESP32 Arduino core 1.0.4. Sloeber is a nice proper Eclipse-based IDE for Arduino development. I could probably dig into the project settings and make it a warning rather than an error, but warnings are scary. Would it be a detriment to any other platform to clarify the code so that certain compilers aren't confused, so that there's is nothing for the compiler to warn about?

The modifications I made do seem to work (and it no longer warns about reorder), I just wanted to make sure I haven't introduced bugs.

obrain17 commented 4 years ago

This not a question of which platform you use (ArduinoIDE, PlatformIO, Sloeber) but which warning level you or the platform select for the gcc or g++ compiler they are all using in common.

If you use -wall ("Alle" in German IDE) you will also get this warnings and error in Arduino IDE. (at least with ESP32)

werror 002

werror 001

An option is to select a lower warning level or at least switch off the reorder being an error, which is possible in some platforms.

In ArduinoIDE you can select Compiler Warnings: Standard

In PlatformIO -w0 or

build_unflags =
   -Werror=reorder

In Sloeber I don't know - but it should be possible, too.

The background of doing the intialization of constructors and members the same order in which they are declared in the class is discussed in https://stackoverflow.com/questions/1828037/whats-the-point-of-g-wreorder and others of course.

In the given example UnsubscribePacket(const std::string& topic): Packet(UNSUBSCRIBE,0,true), _topic(topic){ would cause no warning that the compiler had to do the reorder ("Packet" before "_topic") and no error even with -wall on ESP32.

leifclaesson commented 4 years ago

Thank you! This is helpful. After reading that article I now understand what reorder means. I honestly somehow didn't think to just google it.

I'll just chalk it up to Pangolin being a beta release, then. I personally like to get rid of warnings in my own code as I'm writing it, as normally compiling with 0 warnings, 0 errors make it so much easier to see new warning.

There are certainly compiler settings but I don't know if it's possible to change them per library. Honestly it'll be easier to just keep my own branch to compile against (the beauty of github!) until Phil's mainline branch does compile without errors. Now that you've taught me how to clear the reorder error while still keeping the same general constructor syntax, I can always make a pull request.

luebbe commented 4 years ago

I was about to write that this:

In the given example UnsubscribePacket(const std::string& topic): Packet(UNSUBSCRIBE,0,true), _topic(topic){ would cause no warning that the compiler had to do the reorder ("Packet" before "_topic") and no error even with -wall on ESP32.

would be the correct way to fix the warning. And @leifclaesson please create) the PR :)