Various code optimizations #647
Conversation
…etworks Additionally, this commit will now explicitly allow to omit `wifi.password` from the configuration. Leaving it out will be treated as setting it to `null`.
* OTA will be disabled by default if `ota.enabled` is not provided
mkfrey
force-pushed
the
develop-v3-refactor
branch
from
60eeca5
to
60ca263
Compare
|
Force pushed the last commit to replace |
There was a problem hiding this comment.
Hi @codefrey!
Thank you very much for your work, we really appreciate it! All your proposed changes seem fine, although I don't know what to say about the "delete safeConfigFile" call. I would like to know what @kleini says about it 😉
Also, I don't know if @davidgraeff wants to review the changes 👍
| @@ -472,7 +472,7 @@ void BootNormal::_advertise() { | |||
| { | |||
| char* safeConfigFile = Interface::get().getConfig().getSafeConfigFile(); | |||
| packetId = Interface::get().getMqttClient().publish(_prefixMqttTopic(PSTR("/$implementation/config")), 1, true, safeConfigFile); | |||
| free(safeConfigFile); | |||
| delete safeConfigFile; | |||
There was a problem hiding this comment.
Can anyone else comment on this? My C++ skills are not good enough to review this specific change, sorry.
There was a problem hiding this comment.
Ok, understood. The safeConfigFile is created in
homie-esp8266/src/Homie/Config.cpp
Line 167 in 60ca263
and 'delete' is the correct method to release memory when objects are created by the "new" expression (https://www.geeksforgeeks.org/delete-in-c/).
LGTM!
|
LGTM |
| return strdup(jsonString.get()); | ||
| char* jsonString = new char[jsonBufferLength]; | ||
| serializeJson(jsonDoc, jsonString, jsonBufferLength); | ||
| return jsonString; |
There was a problem hiding this comment.
It's better than before. But you should never return an allocated piece of memory as raw pointer in modern C++.
|
@davidgraeff I thought about using the String class, but String does not implement move semantics so I kept the pointer. It might make sense to replace it with an unique_ptr though. |
nullaswifi.password. Commit 25f6b8c fixes this.ota.enabledwas treated as optional in the config load code, but not in the validation code. Commit 64ca0ba will makeota.enabledoptional and default to disabled OTA.char[]buffer gets allocated within aunique_ptr, only to be copied and and then discarded at the end of the method. Commit 60ca263 removes this unnecessary step.