Add new version of AddSetting that doesn't require pre-initialized variable #316
Conversation
Codecov Report
@@ Coverage Diff @@
## master #316 +/- ##
==========================================
- Coverage 84.99% 84.35% -0.65%
==========================================
Files 196 198 +2
Lines 26846 27082 +236
==========================================
+ Hits 22818 22845 +27
- Misses 4028 4237 +209
Continue to review full report at Codecov.
|
| @@ -17,7 +17,7 @@ int main() | |||
| config_set.AddSetting<int>("int1") = { 1, 2, 3, 4 }; | |||
| config_set.AddSetting<std::string>("string") = { "a", "b", "cde" }; | |||
| config_set.AddSetting<int>("int2") = { 5 }; | |||
| config_set.AddSetting<double>("double", "A double value!", "-d") = { 1.1, 2.2 }; | |||
| config_set.AddSetting<double>("double", "A double value!", '-d') = { 1.1, 2.2 }; | |||
There was a problem hiding this comment.
just curious... what does this change do?
There was a problem hiding this comment.
Makes it actually compile! The 3rd argument is a char, not a string, so it was previously failing to compile.
tests/config/SettingConfig.cc
Outdated
| TEST_CASE("Test SettingConfig", "[config]") | ||
| { | ||
| emp::SettingConfig config_set; | ||
| config_set.AddSetting<int>("test_non_combo") = {5}; |
There was a problem hiding this comment.
does the 5 have to be in {}’s ... ? I might just not quite be understanding what’s going on here
There was a problem hiding this comment.
It does not have to be in {}s, but they also don't hurt anything
There was a problem hiding this comment.
Ah, I had assumed they were there b/c they had to be there for some obscure reason. Might be worth considering removing just to make the code easier to read the next time someone digs thru it (and they don’t assume there’s an obscure reason they have to be there)
| template <typename T> | ||
| T & AddSetting(const std::string & name, | ||
| const std::string & desc="", | ||
| const char option_flag='\0') |
There was a problem hiding this comment.
Should we add optional arg args_label (or remove it from the fun above?)
There was a problem hiding this comment.
I was just trying to keep things consistent between AddSetting and AddComboSetting. I was assuming AddComboSetting is set up this way to avoid ambiguity in which version is being called (but I haven't confirmed that)
| const std::string & desc="", | ||
| const char option_flag='\0') | ||
| { | ||
| emp_assert(!emp::Has(setting_map, name)); |
There was a problem hiding this comment.
Could we default construct a T t; and then just call the fun above so we just have one impl?
There was a problem hiding this comment.
The issue with that is that then we end up with dynamically allocated memory in some cases but not all. We could take care of that with a flag, but since this file is deprecated anyway I don't know if it's worth bothering
There was a problem hiding this comment.
I'm pretty sure we would still always end up with dynamically allocated memory? The function we're delegating to always news
Empirical/source/config/SettingConfig.h
Line 233 in 6a4b844
What I'm thinking of would look like
template <typename T>
T & AddSetting(const std::string & name,
const std::string & desc="",
const char option_flag='\0')
{
T t;
return AddSetting(name, desc, option_flag, t);
}
}
There was a problem hiding this comment.
Sorry, I didn't explain that clearly. We can't default construct a T because it's passed by reference and if we construct it here it will immediately go out of scope. The way to get around that is to dynamically allocate memory for the value inside the SettingInfo constructor. That's the additional dynamically allocated memory I was referring to.
|
@emilydolson approved and left some comments that might be worth addressing before merge |
|
These fixes are superseded by #414 |
As discussed in #315 . This also fixes the SettingConfig.cc and SettingCombo.cc examples, which were previously not compiling.