Support floats in valid config by rikardfalkeborn · Pull Request #1297 · cppcheck-opensource/cppcheck

rikardfalkeborn · 2018-07-07T19:43:44Z

This PR fixes issue 8651 (it does not add any checks to the cfg though, but I tested the example given in the issue with acos and it seemed to work fine).

The first two commits is just some refactoring of tests and addition of more tests. The third commit adds handling of (i.e., no value added). Previously cppcheck would crash, instead bail out with an error message.

The fourth commit actually fixes the issue. The argument value is passed as a double instead of an int. This solves the mentioned issue. Also increase the precision when printing the error message (I chose some arbitrary number). I'm not sure this is needed, but when I tested the example in issue 8651, it didn't print the decimals so I increased precision). Unfortunately, this commit introduces a compiler warning with gcc I wasn't able to figure out how to remove.

lib/library.cpp:735:78: warning: comparing floating point with == or != is unsafe [-Wfloat-equal]
         if (tok->isNumber() && argvalue == MathLib::toDoubleNumber(tok->str()))

Now that we handle valid ranges for floating point arguments, the fifth commit allows for having non-integer valid ranges in the cfg-files. This is currently not used anywhere. The documentation is updated but I wasn't able to actually build it so it would be great if someone could check if that looks ok.

Finally commit six is just a cleanup by moving an assignment inside an if-clause.

In theory, this PR means large parts of CheckFunctions::checkMathFunctions() could be replaced by adding ranges to cfg-files instead (it would also lead to changing from a warning to an error but that's another discussion).

danmar

Overall I think this looks good. Please just split up Library::isargvalid().

danmar · 2018-07-08T06:43:02Z

 }

-bool Library::isargvalid(const Token *ftok, int argnr, const MathLib::bigint argvalue) const
+bool Library::isargvalid(const Token *ftok, int argnr, double argvalue) const


I am skeptic about converting the integer to a double. It may work or not. I would suggest 2 isargvalid functions.

I think the reason I didn't do this is because I added support for non-integer valid values in cfg files. So even if the argument to the function is an integer, the range it is compared to may or may not be an integer (at least in theory) which would complicate this. AFAiCS, there are no std-functions that use non-integer ranges on inputs so one solution is to remove support for that (essentially skip commit five in the PR). We would then know that the ranges are integers and the range checking would be much simpler. That would still male it possible to check if 1.00001 is passed to acos. Extending the valid range to non-integers could be added when there is need for it. Thoughts?

So even if the argument to the function is an integer, the range it is compared to may or may not be an integer (at least in theory) which would complicate this

true. I guess the Library::isIntArgValid function could check if the expression contains a dot somewhere and if there is, return the result of Library::isFloatArgValid.

rikardfalkeborn · 2018-07-09T19:29:04Z

Done. I had to add casts to MathLib::bigint whenever I needed to call isargvalid(..., ..., 1). Also, the compiler warning is still there, it is needed to handle things like 0, 2: and still be able to call the function with 0.0. I'm not sure this is required but I left it in there.

danmar

thanks! I have some further requests.

danmar · 2018-07-10T06:03:54Z

    std::istringstream istr(ac->valid + ',');
    tokenList.createTokens(istr);
+
+    if (istr.str().find_first_of('.'))


it seems better to use find instead of find_first_of. And the condition is true when there is no dot.

You could check if ac->valid contains the dot before you create the TokenList.

Oops, that didn't work... Fixed and checkingac->validinstead.

danmar · 2018-07-10T06:05:51Z

+        }
+    }
+    for (const Token *tok = tokenList.front(); tok; tok = tok->next()) {
+        if (tok->isNumber() && argvalue == MathLib::toDoubleNumber(tok->str()))


I rather not use floating point equality comparisons. For me it would be fine that we just remove this code, so for floats you must always specify ranges.

danmar · 2018-07-10T06:47:38Z

                               "    <arg nr=\"5\"><valid>:1,5</valid></arg>\n"
+                               "    <arg nr=\"6\"><valid>1.5:</valid></arg>\n"
+                               "    <arg nr=\"7\"><valid>-6.7:-5.5,-3.3:-2.7</valid></arg>\n"
+                               "    <arg nr=\"8\"><valid>0.0:</valid></arg>\n"


maybe there should be a test for a :num range

danmar · 2018-07-10T07:31:20Z

    return false;
 }

+bool Library::isargvalid(const Token *ftok, int argnr, double argvalue) const


hmm.. we now have copy/paste I wonder if we can avoid it without using double for the integers. one thing that can be done is creating a utility function that creates the token list.

I extracted the token list creation to a separate function. There's still a lot of copy/paste but I couldn't figure out a good way to remove it.

amai2012 · 2018-07-10T18:46:32Z

What string formats of the floats are now supported? Just %f?
I'd appreciate a real-life example, i.e. an example based on a real cfg file. It should probably indicate that cppcheck-cfg.rng also requires some modifications
May this trigger overflow/underflow errors, e.g. when using a .cfg from a platform with different numerical limits?

rikardfalkeborn · 2018-07-10T19:54:23Z

Oops, didn't see this before I pushed it.

What string formats of the floats are now supported? Just %f?

Yes. I added this as an example in the docs, is more documentation needed?

I'd appreciate a real-life example, i.e. an example based on a real cfg file. It should probably indicate that cppcheck-cfg.rng also requires some modifications

What kind of real-life example are you looking for? TBH I don't have a real life example where the valid range is not an integer, it just seemed silly to not allow it when the "only" thing needed to support it was parsing of the cfg-files (and apparently the rng file too). I could add a range for e.g. acos from -1.0 to 1.0 and make sure this works as intended (that'd include removing the check of acos fromlib/checkfunctions.cpp). I didn't do this because I figured the PR was big enough already (but I can add this of course). I'll take a look at cppcheck-cfg.rng though.

May this trigger overflow/underflow errors, e.g. when using a .cfg from a platform with different numerical limits?

Not sure I follow. Can you elaborate?

rikardfalkeborn · 2018-07-10T20:36:12Z

It should probably indicate that cppcheck-cfg.rng also requires some modifications

Fixed in the last push.

amai2012 · 2018-07-10T20:51:19Z

It feels strange, that no exponential format is accepted. Therefore it should be noted IMHO.
As for overflows: What is going to happen if 1e999 (or equivalent) is specified in .cfg on a platform with DBL_MAX~1e308?
Yes, the additional example was meant to verify that things work in real life, and not just in this kind of unit testing.

rikardfalkeborn · 2018-07-12T20:21:40Z

It feels strange, that no exponential format is accepted. Therefore it should be noted IMHO.

Implementing support for exponential format (e.g. 1e10) is an additional four lines of code (plus tests) so I'd rather to that instead of adding a note if you prefer it?

As for overflows: What is going to happen if 1e999 (or equivalent) is specified in .cfg on a platform with DBL_MAX~1e308?

All comparissons are done with the return value ofMathLib::toDoubleNumber() so it depends on how that function handle it. A quick test shows that on my system (Linux, Gcc), setting the limit to:1e999 makesisargvalid() return true forstd::numeric_limits<double>::max()). I'm unsure if this is true for all systems or if we're in implementation-defined territory here.

Yes, the additional example was meant to verify that things work in real life, and not just in this kind of unit testing.

Good point. I'm unsure how to proceed though. Creatingtest.cfgto add some dummy functions to trigger cornercases?

danmar · 2018-07-12T20:45:15Z

Good point. I'm unsure how to proceed though. Creating test.cfg to add some dummy functions to trigger cornercases?

I'd say remove the hardcoding for acos, add the proper configuration in std.cfg for acos. Then see if the TestFunctions::mathfunctionCall_acos works as it should. It's not necessary to output identical error message as before but it should be similar in spirit.

rikardfalkeborn · 2018-07-14T07:11:48Z

Good point. I'm unsure how to proceed though. Creating test.cfg to add some dummy functions to trigger cornercases?

I'd say remove the hardcoding for acos, add the proper configuration in std.cfg for acos. Then see if the TestFunctions::mathfunctionCall_acos works as it should. It's not necessary to output identical error message as before but it should be similar in spirit.

Former error message: "[test.cpp:3]: (warning) Passing value -1.1 to acos() leads to implementation-defined result.\n"
New error message: "[test.cpp:3]: (error) Invalid acos() argument nr 1. The value is -1.1 but the valid values are '-1:1'.\n"

So quite similar in spirit, but the severity is changed.

danmar · 2018-07-15T08:50:39Z

that looks good. If the argument is invalid it sounds reasonable with error severity. Ideally the valid expression is written in english instead somehow, but that is not your problem now.

rikardfalkeborn · 2018-07-15T11:17:20Z

that looks good. If the argument is invalid it sounds reasonable with error severity. Ideally the valid expression is written in english instead somehow, but that is not your problem now.

I'll add a commit to that then. What about being specify ranges as 1.0 or 1e4, should I keep/add support for that? Since there is no users of it it'll be somewhat untested. What do you think?

danmar · 2018-07-15T11:42:02Z

imho you can skip 1e4 to start with. if we need that later it can be added then.

This reduces the amount of code slightly and will simplify adding more tests.

Before this change, the sequence <valid></valid> in a config file would result in a segmentation fault. Now an empty field results in the error message: cppcheck: Failed to load library configuration file 'mycfg.cfg'. Bad attribute value '""'

Previously, it was not possible to add valid ranges to floating point arguments since it only handled integers. This made ranges not work well for floating point arguments since arguments were cast to integers before the ranges were handled. Fix this by using doubles instead of integers if the argument is a float. Add some tests for this and make sure errors are printed with enough precision (somewhat arbitrarily chosen). Note that it is still only possible to add integer ranges (i.e. -1:1).

Now that it is possible to handle decimal arguments, there is no reason to not allow non-integer ranges. Take care to not allow broken configurations.

rikardfalkeborn · 2018-07-15T18:32:00Z

New push with asin/acos valid range moved to cfg-file. More functions can be moved like this, but I think this is enough for this PR?

danmar · 2018-07-15T20:47:51Z

thanks

danmar requested changes Jul 8, 2018

View reviewed changes

rikardfalkeborn force-pushed the support-floats-in-valid-config branch from ab260f8 to c525db6 Compare July 9, 2018 19:25

danmar requested changes Jul 10, 2018

View reviewed changes

rikardfalkeborn force-pushed the support-floats-in-valid-config branch from c525db6 to 9730597 Compare July 10, 2018 19:34

rikardfalkeborn force-pushed the support-floats-in-valid-config branch from 9730597 to be29e89 Compare July 10, 2018 20:35

rikardfalkeborn added 7 commits July 15, 2018 18:56

Refactor loadLibErrors

d05e224

This reduces the amount of code slightly and will simplify adding more tests.

Add tests for invalid ranges

1c9f46c

Handle empty valid field

ff46ff1

Before this change, the sequence <valid></valid> in a config file would result in a segmentation fault. Now an empty field results in the error message: cppcheck: Failed to load library configuration file 'mycfg.cfg'. Bad attribute value '""'

Add support for floats in configuration valid range

34e5ba4

Now that it is possible to handle decimal arguments, there is no reason to not allow non-integer ranges. Take care to not allow broken configurations.

Move check to within if-clause

30dea89

Move asin{,f,l} and acos{,f,l} input checks to config file

e6383b9

rikardfalkeborn force-pushed the support-floats-in-valid-config branch from be29e89 to e6383b9 Compare July 15, 2018 18:29

danmar merged commit 491ee57 into cppcheck-opensource:master Jul 15, 2018

rikardfalkeborn deleted the support-floats-in-valid-config branch July 15, 2018 21:52

Conversation

rikardfalkeborn commented Jul 7, 2018

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rikardfalkeborn commented Jul 9, 2018

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amai2012 commented Jul 10, 2018

Uh oh!

rikardfalkeborn commented Jul 10, 2018

Uh oh!

rikardfalkeborn commented Jul 10, 2018

Uh oh!

amai2012 commented Jul 10, 2018

Uh oh!

rikardfalkeborn commented Jul 12, 2018

Uh oh!

danmar commented Jul 12, 2018

Uh oh!

rikardfalkeborn commented Jul 14, 2018

Uh oh!

danmar commented Jul 15, 2018

Uh oh!

rikardfalkeborn commented Jul 15, 2018

Uh oh!

danmar commented Jul 15, 2018

Uh oh!

rikardfalkeborn commented Jul 15, 2018

Uh oh!

danmar commented Jul 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants