Make sure incorrect conversions trigger an exception #722
Conversation
|
Note: there's something wrong with GitHub actions tests on Linux, I have to check them... |
Codecov Report
@@ Coverage Diff @@
## master #722 +/- ##
==========================================
+ Coverage 85.56% 85.60% +0.03%
==========================================
Files 588 588
Lines 47670 47680 +10
==========================================
+ Hits 40790 40815 +25
+ Misses 6880 6865 -15
Continue to review full report at Codecov.
|
I changed the code in private convertTo* functions to the original form (return bool). This is actually faster when exceptions are raised frequently. In this manner, I solve the performance issue on Linux.
This reverts commit 0ed75e9.
This reverts commit 8f0d062.
In the original fix I was making convert() throw exceptions. However, we use a lot failed conversions when reading inputs. Since catching an exception is much more expensive than inspecting a returned bool, this was slowing down a lot some parts of the code In 0ed75e9 I solved the issue by allowing to throw only a few functions. Here I take a radically different approach: a new function (convertNoexcept) can be used like to old one, and returns a bool. The function convert just calls convertNoexcept and, if false, throws. In this manner there is no overhead expected at all with respect to the previous version of the code. In addition, the modifications are less intrusive. It is sufficient to replace convert with convertNoexcept in all the places where the returned bool was checked
|
After having seen that there is a performance penalty in catching exception, I made an alternative implementation where exceptions are only used when needed. In the new implementation:
The advantage with respect to the previous implementation is that we only throw exceptions when the is a non-detected error. We can still repeatedly call The changes to the code are also simpler since there's no need to add @gtribello there is still this point to double check. In order not to break a test, I am ignoring the result here. |
This is an attempt to fix #717. I am applying it directly to version 2.8 but it might be backported.
Description
OUTDATED, see below
I modified the way the functions
Tools::convert()work. Before this change, they were returning a bool (false meant an error). This is a poor design, since the user might ignore the error easily, and was used since in the first PLUMED 2 releases we were not using C++ exceptions yet. After this change, these functions return void and raise an exception in case of an error. This makes it more difficult to inadvertently ignore conversion errors.Basically, these modifications required me to modify all the places where the return value was correctly checked. All these places were immediately detectable as compilation error since the return value of these functions was accessed. For instance, something like this:
now would not compile unless it is converted to:
In fixing the code I had to adjust some checks in VES and OPES (cc: @valsson and @invemichele).
In addition, I added a workaround to the code since there's a place where conversions were not checked and should not be checked to pass current regtests. More precisely, the original code was not checking the returned value, and, consistently, the new code ignore any exception triggered here. @gtribello please check if the fix is ok or if there's something else to do here. In particular, I think that to pass test
multicolvar/rt-dens-averageit would be sufficient to allow for empty strings (rather than to allow for arbitrary strings as it is now).Target release
I would like my code to appear in release v2.8 but could be backported
Type of contribution
Copyright
COPYRIGHTfile with the correct license information. Code should be released under an open source license. I also used the commandcd src && ./header.sh mymodulenamein order to make sure the headers of the module are correct.Tests