ConfigTree with some checks added #912
Conversation
|
Looks OK. I think that to truly find out if the design is good an exemplary change to any of the current config parsers would be helpful. Maybe you could choose one particularly ugly example :) |
| * is generated. The message contains a hint where it occured. | ||
| * * enforces a naming scheme of settings: letters a-z, numbers 0-9, underscore | ||
| * * provides some functionality to read lists of values using range-based for loops. | ||
| * * has rater lng method names that are easily greppable from the source code. So a list |
|
Good job, go on! |
|
Telling from this thread: http://stackoverflow.com/questions/10783391/is-it-possible-to-move-a-boostoptional See also: http://www.boost.org/doc/libs/1_60_0/libs/optional/doc/html/boost_optional/relnotes.html |
|
I haven't any objections to require a higher boost version. |
|
Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/891/ |
|
Jenkins test this please. It was just the DenseMatrix test again... |
|
Cool stuff! 👍 |
| markVisited(root); | ||
| boost::optional<ConfigTreeNew> t; | ||
| t.emplace(*subtree, *this, root); | ||
| return std::move(t); |
There was a problem hiding this comment.
BaseLib/ConfigTreeNew.cpp:99:16: warning: moving a local object in a return statement prevents copy elision
clang's suggestion: note: remove std::move call here
@chleh could you explain why making t an rvalue is needed?
There was a problem hiding this comment.
No, I can't, will remove it.
ConfigTree with some checks added
| ConfigTreeNew:: | ||
| getConfParamOptional(std::string const& param) | ||
| { | ||
| checkUnique(param); |
There was a problem hiding this comment.
Why are the other getters (getConfParam and its optional version) without this checkUnique?
There was a problem hiding this comment.
The other methods call this one, so additional checks are not necessary.
|
I'd recommend to have consistent tabs/whitespaces at least in a single file. (Or use clang-format) |
|
Since this PR has already been merged, I will fix the indentation in a subsequent PR. |
|
|
||
| if (subtree) { | ||
| markVisited(root); | ||
| return boost::optional<ConfigTreeNew>(std::move( |
There was a problem hiding this comment.
Same as for the other move: copy-elision with std::move is not possible. I think the return statement could be just
return ConfigTreeNew(*subtree, *this, root);|
OpenGeoSys development has been moved to GitLab. |
This PR introduces a wrapper class for the Boost Property Tree. The purpose of the wrapper is:
Those points are basically implemented by keeping track of how many times a routine asks for a certain parameter. The destructor then will check that everything has been read.
Currently the changes in this PR are somewhat scattered. i will clean it up later on. I believe that everything we need can be done with the new class and that a transition is easy.
The main part (interface) of this PR is in
BaseLib/ConfigTreeNew.h. It is used pretty much in the same way as the Boost property tree (cf.Tests/BaseLib/TestConfigTree.cpp). Please have a look if any feature is missing or if you disagree with my implementation.A typical waring message will look like this:
ConfigTree: At path <OpenGeoSysProject/processes/process/linear_solver>: Key <eigen> has been read 1 time(s) less than it was present in the configuration tree.(of cause this particular message will not occur.)
In this PR the options of
ProcessVariables are parsed with the newConfigTree. Once this PR is accepted I will make the transition to the new ConfigTree in all of OGS-6.There is one design question to be discussed: Currently the new ConfigTree cannot be passed as
const &but only as reference to a modifiable object. That can be changed if I make one member variablemutable. Should I do that?