Reflections on the code
Moderator: Forum Moderators
-
- Posts: 27
- Joined: November 20th, 2020, 5:07 am
Reflections on the code
Here is my first impression of the few fragments of code that I have read.
/src/config.hpp: class config has two
/src/config.hpp: When
The
method takes in a const reference to a node, copy it, and return the copy as a non-const reference.
Would the following work?
The new version has less duplication of code.
But I worry that the new version might be slower because the move constructor of
C++14 and copy elision: https://en.cppreference.com/w/cpp/language/copy_elision
There are two possible ways to test if the new version is better:
Debug macros can do that but I don't like them because they can cause heisenbug.
Benchmark has the final say in performance. However, to be sure, I need to run the benchmark on all supported
platforms and supported compilers. A new set of build jobs on Travis or Github Action can do that.
/src/config.hpp: class config has two
child
methods.
- One of the method raises an exception when it can't find the child.
- The other method returns the static invalid node when it can't find the child.
config::require_child
seems a better name for the method that raises an exception.config.hpp fragment:
config::child_or_empty
can't find a child, it returns an empty node singleton instead of the static invalid node:
Do we need both the empty node singleton and the static invalid node? I have to read through the many usages of this method to find out.
/src/config.hpp: class config contains the definitions of multiple nested classes, which are quite difficult to read:
config.cpp fragment:
Only declare the inner classes within class config may make class config more readable.
https://stackoverflow.com/questions/448 ... ource-file
/src/config.cpp: class config has three very similar https://stackoverflow.com/questions/448 ... ource-file
config.hpp fragment:
config::add_child
:
config.cpp fragment:
config& config::add_child(config_key_type, const config&)
method takes in a const reference to a node, copy it, and return the copy as a non-const reference.
Would the following work?
new version:
But I worry that the new version might be slower because the move constructor of
class config
is not free.C++14 and copy elision: https://en.cppreference.com/w/cpp/language/copy_elision
There are two possible ways to test if the new version is better:
- Count the number of copy-construction & move-construction of config.
- Benchmark
Debug macros can do that but I don't like them because they can cause heisenbug.
Benchmark has the final say in performance. However, to be sure, I need to run the benchmark on all supported
platforms and supported compilers. A new set of build jobs on Travis or Github Action can do that.
Last edited by CrawlCycle on November 22nd, 2020, 2:58 pm, edited 18 times in total.
- Pentarctagon
- Project Manager
- Posts: 5564
- Joined: March 22nd, 2009, 10:50 pm
- Location: Earth (occasionally)
Re: Reflections on the code
child_or_throw
would be another option.99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
take one down, patch it around
-2,147,483,648 little bugs in the code
- Celtic_Minstrel
- Developer
- Posts: 2214
- Joined: August 3rd, 2012, 11:26 pm
- Location: Canada
- Contact:
Re: Reflections on the code
I think
child
should be called get_child
if we're going to rename it at all, because that's the name of the Lua function that does the same thing. As for the static invalid node, I'd like to see it removed entirely, as it makes it harder to report comprehensible errors.