Reflections on the code

Discussion of all aspects of the game engine, including development of new and existing features.

Moderator: Forum Moderators

Post Reply
CrawlCycle
Posts: 27
Joined: November 20th, 2020, 5:07 am

Reflections on the code

Post by CrawlCycle »

Here is my first impression of the few fragments of code that I have read.

/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:
/src/config.hpp: When 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.
config.cpp fragment:
/src/config.hpp: class config contains the definitions of multiple nested classes, which are quite difficult to read:
Only declare the inner classes within class config may make class config more readable.
https://stackoverflow.com/questions/448 ... ource-file
config.hpp fragment:
/src/config.cpp: class config has three very similar config::add_child:
config.cpp fragment:
The 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:
The new version has less duplication of code.
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:
  1. Count the number of copy-construction & move-construction of config.
  2. Benchmark
Counting the number of construction seems to require modifying the main source to count constructions.
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.
User avatar
Pentarctagon
Project Manager
Posts: 5526
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Reflections on the code

Post by Pentarctagon »

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
User avatar
Celtic_Minstrel
Developer
Posts: 2166
Joined: August 3rd, 2012, 11:26 pm
Location: Canada
Contact:

Re: Reflections on the code

Post by Celtic_Minstrel »

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.
Author of The Black Cross of Aleron campaign and Default++ era.
Former maintainer of Steelhive.
Post Reply