Remove unneeded optional from ParserResult #2042
Conversation
This change is based upon a toy project I have at https://github.com/Molter73/config-much. The basic idea is to use the reflection API from protobuf to populate the runtime configuration structures, which should allow us to add new fields to our protobuf definitions and have them magically show up in our code to use.
There was a problem hiding this comment.
Hey @JoukoVirtanen! Thanks for putting this PR together.
I appreciate that removing the std::optional from ParserResult simplifies access to it very slightly, but as I mentioned in my other comment, it does so by being less idiomatic C++17 and making it harder to convey its intention, checking if there are errors is a lot easier to read with if (errors) than if (!errors.empty()), which is a C++11 idiom that was used because there was no better alternative.
With that said, if it's up to me, I wouldn't move forward with this change.
| @@ -488,9 +480,9 @@ ConfigLoader::Result ConfigLoader::LoadConfiguration(const std::optional<const Y | |||
| errors = parser_.Parse(&runtime_config, *node); | |||
| } | |||
|
|
|||
| if (errors) { | |||
| if (!errors.empty()) { | |||
There was a problem hiding this comment.
I don't really like this, if (errors) { is a much more idiomatic way to express that you will execute something if errors happened when compared to if (!errors.empty()) {. Performance wise, both options should be the same, because we create vectors in the inner methods, so not much difference there and this code is not in any hot path, so I wouldn't worry about it.
Molter73
force-pushed
the
mauro/runtime-config-reflection
branch
from
62700c0 to
8e71a90
Compare
Description
Removes optional from
using ParserResult = std::optional<std::vector<ParserError>>;and makes other changes so that things are consistent.Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Unit tests should be sufficient.