Allow user to specify seconds-only wall time in either S or Ss format #1231
Conversation
github-actions
bot
added
AT: WIP
|
CLANG-FORMAT TEST - FAILED (on last commit): |
github-actions
bot
added
AT: CMAKE-FORMAT PASS
and removed
AT: WIP
|
CMAKE-FORMAT TEST - PASSED |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
github-actions
bot
added
AT: WIP
|
CLANG-FORMAT TEST - PASSED |
github-actions
bot
removed
the
AT: WIP
|
CMAKE-FORMAT TEST - PASSED |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
| catch ( const std::exception& e ) { | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I would write it:
uint32_t
parseWallTimeToSeconds(const std::string& arg, bool& success, const std::string& option)
{
// first attempt to parse seconds only. Assume \d+[s] until it's not
char* pos;
auto seconds = strtoul(arg.c_str(), &pos, 10);
while ( isspace(*pos) )
++pos;
if ( *pos == 's' || *pos == 'S' ) {
while ( isspace(*++pos) )
;
if ( !*pos ) {
if ( seconds <= UINT32_MAX ) {
success = true;
return seconds;
}
}
}- It avoids memory allocation by not creating
str - It avoids exceptions by using C
strtoulinstead of C++std::stoul - It allows
sorSsuffix - It allows spaces before and after
sorS - It falls back to the old code if it doesn't match Perl regexp
^\s*[0-9]*\s*[Ss]\s*$or is out of range ofuint32_t
There was a problem hiding this comment.
acknowledged - will make these updates
github-actions
bot
added
AT: WIP
|
CMAKE-FORMAT TEST - PASSED |
|
CLANG-FORMAT TEST - PASSED |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
github-actions
bot
added
AT: WIP
|
CLANG-FORMAT TEST - PASSED |
github-actions
bot
added
AT: WIP
|
CMAKE-FORMAT TEST - PASSED |
|
CLANG-FORMAT TEST - PASSED |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
rebased |
src/sst/core/configBase.cc
Outdated
| @@ -59,14 +59,29 @@ ConfigBase::parseBoolean(const std::string& arg, bool& success, const std::strin | |||
| uint32_t | |||
| ConfigBase::parseWallTimeToSeconds(const std::string& arg, bool& success, const std::string& option) | |||
| { | |||
| // first attempt to parse seconds only. Assume \d+[s] until it's not | |||
| char* pos; | |||
| auto seconds = strtoul(arg.c_str(), &pos, 10); | |||
There was a problem hiding this comment.
Change 'auto' to 'unsigned long'. Otherwise looks good.
There was a problem hiding this comment.
<rant>
The reason auto was originally used, was to put the type declaration in one place, namely the return value of the function (strtoul, strtoull, etc.), and to prevent differences between the declared type of the variable and the return type of the function, which could lead to truncation or other unexpected behavior.
Using auto makes the type of a variable, or the return type of a function, etc., be the same as the value it is being set to, returning, etc., when knowing the exact type used is less important than the meaning of the variable or function. C++14 adds auto to function return types and generic lambdas; C++17 adds auto for non-type template parameters, so that you can write a template which performs the same operation on an entity regardless of its type; and C++20 adds auto for function parameters.
Some people say it's confusing, but I actually think it's clearer, when the variable, function or template parameter is "more important in function than in form". In the example cited here, the variable is declared auto because its precise type is not central to the operation, and the code says "Give me a variable which can hold the result of strtoul() or some other function which performs a certain operation. I don't care about its type, just the operational function. If later I decide to change it to strtoull() or some other function, the variable will follow the function."
Using auto is also to shield yourself from changes in an external library, or to use opaque types whose implementation details or exact layout is not important. You write your code once with auto, and if the function, variable, etc. changes, you don't have to change your code as long as it still supports the same functions/methods/etc. on the type.
Using auto is similar to using let in functional languages, where the operation is more important than the type.
Now, when doing things like writing code for processor registers and instructions, I don't use auto because the instructions and registers have a very specific size. For those, I use types like uint32_t and uint64_t rather than auto or even unsigned long.
There was a problem hiding this comment.
I've read a lot of online discussions on this topic and there seems to be people on both sides of this. Many think auto should be used everywhere for many of the reasons you describe. Others argue auto should only be used when necessary, and not out of convenience, to enhance code understandability. I can see both sides of the argument, but for SST, we've fallen more towards the later. In my mind, most of what you describe above is convenience, not necessity. It is convenient not to have to change my code if I change the type of a variable. On the other hand, if I go and change the type everywhere in the code, then people reading the code will know what types they're dealing with. It is convenient not to have to change my code if an external library changes. On the other hand, if an external library changes in a way visible to my code, I'd want to review my code anyway to make sure it's still interacting in a proper way. For how rarely these types of things happen, I find the extra work to be a good trade-off for better accessibility to developers (especially new developers, many of whom are not C++ gurus).
For me, having a specific type instead of auto would never be less understandable, though it may not always be more understandable. In C++, form and function are often linked, i.e., you can't understand the code without knowing the types you're operating on. The cost of entry to understanding a new code base goes up dramatically when auto is used everywhere because understanding the data you're operating on becomes challenging. In SST we intentionally decided to err on the side of readability over convenience in the hopes of being able to enable more users to contribute. I do recognize this can cause some pain in writing code. I also recognize that there are cases where auto could help with things like avoiding truncation I that is something we give up with this standard, and we could look at refining the way we use auto to account for that. Though I typically see compiler warnings if I haven't specifically downcast the bit width.
As to this specific case, in the end, I think either auto or unsigned long are both as readable. But, where do you draw the line? What's easily understood by one may not be easily understood by another. In developing SST, we have decided not to use auto except when it's necessary. We also make exceptions for things like iterators, but it's best to only use it there if the types you're dealing with are immediately recognizable in the surrounding code. If there's no way to do what you want without using auto, then a comment about how to determine types would be welcome.
…either S or Ss format
github-actions
bot
added
AT: WIP
|
CLANG-FORMAT TEST - FAILED (on last commit): |
github-actions
bot
removed
AT: CLANG-FORMAT PASS
AT: WIP
|
CMAKE-FORMAT TEST - PASSED |
|
changed auto to unsigned long and retested locally. |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
github-actions
bot
added
AT: WIP
|
CLANG-FORMAT TEST - PASSED |
|
CMAKE-FORMAT TEST - PASSED |
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore
Using Repos:
Pull Request Author: kpgriesser |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ feldergast ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge |
This PR allows the user to specify seconds-only wall time in either S or Ss format ( e.g. >60s ). Fixes issue #1165.
ConfigBase::parseWallTimeToSeconds(...) is modified to first check if the argument is of the form S or Ss and parses the value directly. If that fails the normal path is followed.