[GeoMechanicsApplication] Fix std::format code smells #13750
Conversation
…, which I can't figure out how to combine with std::format)
| << "value of " << rVariable.Name() << " for property " << rMaterialProperties.Id() | ||
| << " is out of range: " << rMaterialProperties[rVariable] << " is not in [0.0, " | ||
| << (MaxValue ? std::to_string(*MaxValue) + "]" : "->") << std::endl; | ||
| << std::format("value of {} for property {} is out of range: {} is not in [0.0, {}]", |
There was a problem hiding this comment.
To me it seems that now your new message adds a closing bracket like [0.0, ->].
That reads better, but I'm not sure if this was intentional or not?
There was a problem hiding this comment.
It can not be intentional, the open interval boundary is -> a closed one is "value"]
There was a problem hiding this comment.
It wasn't, my mistake, fixed now.
The weird part is this function is quite heavily tested, but even with the extra bracket, this still passed:
I double-checked and it turns out KRATOS_EXPECT_EXCEPTION_IS_THROWN checks if the message is there as a substring (meaning if there are extra things around a message, the test still passes).
| std::string progress = "Calculating head level " + current_head_string + "m (" + | ||
| std::to_string(step) + "/" + std::to_string(max_steps) + ")"; | ||
| const auto progress = | ||
| std::format("Calculating head level {:.8}m ({}/{})", mCurrentHead, step, max_steps); |
There was a problem hiding this comment.
Is the std::setprecision(8) now the same as: std::format(...{:.8}...)?
I was looking it up a bit, and I saw that if you don't specify it, it does general formatting (so g).
I guess the intention is not to use fixed-point with exactly 8 digits after the decimal? (so {:.8f})
There was a problem hiding this comment.
Indeed, like this it works the same as setting the precision (I did some experimentation and both give the same results). When adding the f we indeed always get that precision, so 3.2 would become 3.20000000, which is different from the current functionality (also the related unit tests started failing when I tried this)
…der somehow leads to build issues on ubuntu
| std::string progress = "Calculating head level " + current_head_string + "m (" + | ||
| std::to_string(step) + "/" + std::to_string(max_steps) + ")"; | ||
| const auto progress = | ||
| std::format("Calculating head level {:.8}m ({}/{})", mCurrentHead, step, max_steps); |
There was a problem hiding this comment.
Indeed, like this it works the same as setting the precision (I did some experimentation and both give the same results). When adding the f we indeed always get that precision, so 3.2 would become 3.20000000, which is different from the current functionality (also the related unit tests started failing when I tried this)
| << "value of " << rVariable.Name() << " for property " << rMaterialProperties.Id() | ||
| << " is out of range: " << rMaterialProperties[rVariable] << " is not in [0.0, " | ||
| << (MaxValue ? std::to_string(*MaxValue) + "]" : "->") << std::endl; | ||
| << std::format("value of {} for property {} is out of range: {} is not in [0.0, {}]", |
There was a problem hiding this comment.
It wasn't, my mistake, fixed now.
The weird part is this function is quite heavily tested, but even with the extra bracket, this still passed:
I double-checked and it turns out KRATOS_EXPECT_EXCEPTION_IS_THROWN checks if the message is there as a substring (meaning if there are extra things around a message, the test still passes).
|
@KratosMultiphysics/geomechanics Unfortunately, I was a bit too early with this PR: before we can start using For reference, see https://en.cppreference.com/w/cpp/20.html |
📝 Description
After the upgrade, static analysis suggests to change a couple of places where string concatenation is done, which can now be improved by using std::format.