Sat, 05/05/2012 - 17:37
Hello,
while using the trial version of a static analysis product, I tryed it with OCE to evaluate it with a very big solution :)
Surely I can't disclose the result log, since it won't be ethically correct , but I'm reporting the problem. Probably clang or MSVC /analize could report similar issues.
Apart the usual false positives, there is a HUGE quantity of float comparisons done without a tolerance range (i.e. using operator ==).
With HUGE quantity i mean almost everywhere (~1500 possible entries)...
While in some cases it can be ok (i.e. checking some pre-assigned values), most cases require to check against a tolerance (fabs(A)
For example
Standard_Real delt1 = Abs(prml-prmf);
Standard_Real delt2 = Abs(period-delt1);
if (delt1 == 0 || delt2 == 0)
{....
you surely know better than me that this check is a potential bug. Also the result is influenced by OS,machine and compiler settings.
--
Three questions:
- What is the appropriate function/functions to use to check the values for 0 or for equality?
- I know you usually work on a per-case basis (it is also stated somewhere in this forum). But, do you plan to do something for these global issues?
- In my small experience, the tolerance used to do these checks can influence the whole result of an algorithm. While using a very small precision (i.e. Precision::Confusion) can at least solve the "float comparison issue", using a precision related to the kind of value you are checking can be better or more appropriate. If you are checking a sqrt delta, it is very different from checking two vertex position values, and maybe two different precisions should be used. Also, checking a "divisor" with a too small tolerance, may lead to a very big floating number which results in a loss of precision if used in subsequent operations.
Is there a "use-pattern" in OCC for this?
Surely solving this could give OCC a stability boost, but could also mess up things as potential breaking change!
QbProg
p.s. I didn't know where to put this topic, so I choose the roadmap section,maybe one section for development discussions could be added?
Thu, 05/10/2012 - 19:46
Hello,
Sorry for delay in answer -- we had long holidays here..
(For the place for this and similar topics, I deem Road-map section is reasonable choice, at least with the current small amount of topics.)
Regarding comparison of reals by ==:
0. No doubt it is bad practice, and we are trying to eliminate such cases when identified in our routine activity.
1. The common practice to check the value for being zero (or two values to be equal) -- by comparing the absolute value (difference) with some precision -- is in plain code, i.e. with no special function employed. I believe this approach is appropriate. Note that some classes (e.g. gp_Pnt) provide methods (typically called IsEqual) to check for equality.
2. Currently we do not plan to dedicate special efforts to dealing with this kind of issues, as it is difficult to expect touchable improvements from corrections made without real test cases.
3. The precision to be used in particular check generally depends on the nature of the quantities being compared.
OCCT provides some default precision values for comparison of 3d co-ordinates (Precision::Confusion), parametric co-ordinates (Precision::PConfusion), angular values (Precision::Angular), etc.
It is clear that these values are just default 'stub' and in each particular case some better defined value can be used. For instance, in the code dealing with topological shapes tolerance is typically used for coincidence checks, as tolerance is a measure of precision of the geometric representation of the entity. When working with parametric geometries, 3d tolerance can be mapped to parametric space using so-called 'Resolution' methods (estimates of maximum moduli of derivatives). In low-level math functions, precision is often chosen basing on floating point resolution, etc.
In practice, it is not always possible (or feasible) to choose really 'true' precision value for each check. Hence in many places you can observe usage of seemingly doubtful values, such as direct comparison to zero. I believe, however, that most of such cases are 'safe', i.e. they either check for equality to special values, or serve as protection against singular cases which should not happen in normal use.
I agree that cleaning the code from obviously wrong comparisons, such as those you have reported above, should theoretically improve the code. Note though that direct comparisons of reals is just the most obvious, by unlikely the most important situation when precision of calculation can be lost. Practically each floating-point calculation can be suspect of possible precision problems, depending on the input data. Hence I do not see alternative to our current case-based approach: the problem is fixed when it is identified.
This is where we would expect help from the user's community: it should not be difficult for everyone who spots this kind of problem to report it in the bug tracker and possibly provide a correction via Git. Now we have infrastructure allowing us to run certification tests and integration of code changes efficiently, thus your contributions are welcome!
Andrey