Add script to filter Doxygen warnings #3250

johnnonweiler · 2018-10-30T12:11:22Z

Adds a list of expected warnings from Doxygen and a Python script to filter out those warnings from the Doxygen output.

The list of expected warnings includes all the warnings that I currently see when running Doxygen (and no others).

In some cases, it would be relatively easy to fix the Doxygen documentation so that we don't get these warnings. (And I do believe that these should be fixed and then removed from the list.) However, in other cases there are bugs in Doxygen, where the code and documentation are perfectly valid and Doxygen gives an erroneous warning message.

Currently, CI uses another script to filter out warnings related to lines of code which have not been changed. However, this doesn't work well because the line number reported for a warning is often not the line where the problem lies.

The new script should be useful for CI, and also for checking changes locally before running CI.

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

allredj

🚫
This PR failed Diffblue compatibility checks (cbmc commit: 37612fb).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89648149
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

hannes-steffenhagen-diffblue

I think this should also fail if there's a warning in the list of expected warnings that doesn't actually show up in the list of real warnings. IMHO would be better to not store the things we don't need (like full paths) in the first place.

I thought about having a script that generates the list in here, but since we really shouldn't be adding to this list I think it's fine.

scripts/filter_expected_warnings.py

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 92e90ba).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89955533

scripts/filter_expected_warnings.py

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: c7cb2ef).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89962127

hannes-steffenhagen-diffblue · 2018-11-01T16:18:46Z

scripts/filter_expected_warnings.py

-            for line in warning.otherlines:
-                print('  ' + line)
+    unexpected_warning_found = list1_is_in_list2(
+        new_warnings, expected_warnings, '')


I considered that, but I think it's better to report warnings in the same way as normal in the case where new problems have been introduced and old ones haven't been fixed.

scripts/filter_expected_warnings.py

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: d3a33d0).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89969637

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 68fd12d).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89975506

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 08a9d7d).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89981745

johnnonweiler · 2018-11-02T11:38:04Z

I've now also added scripts/run_doxygen.sh to make it easier to run doxygen using this filter.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 49513f0).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90067721

johnnonweiler · 2018-11-02T13:43:43Z

I've just rebased and retested this on the latest develop, and had to add six more warnings to the list of expected warnings, and remove one warning which has been fixed. (This demonstrates that the current CI check fails to stop people introducing new problems.)

However, there are 28 new warnings which I have not included in the list of expected warnings, because the filter can't handle them and I think they should be fixed instead. They all stem from the fact that doc/cprover-manual/assumptions.md is identical to doc/cbmc-user-manual.md. (Again, the current CI job missed this.)

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 4558e21).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90081552

owen-mc-diffblue

I've pointed out two very minor things, but I'm happy for it to go in as is.

owen-mc-diffblue · 2018-11-02T15:25:25Z

scripts/filter_expected_warnings.py

+                warning = matched.group(2)
+                current = DoxygenWarning(line.strip(), filename, warning)
+                self.warnings_list.append(current)
+            elif line.startswith('  '):


⛏️ It would make more sense to me if you would check if the line starts with ' ' first, and only if it doesn't would you try the regular expression, and if that doesn't match then you'd report an error. It would reduce the number of times you try the regular expression. But you said it runs very fast, so it's not important.

scripts/filter_expected_warnings.py

thk123

Needs referencing from existing docs.

I'm not completely sold on merging without it being used in CI but accept it might be useful for next doc day, so if no time to integrate into CI by then then happy for this to be merged (though has the downside that if CI is doing different check to local, then you might think your thing will pass, but it in fact will not).

If you wanted to make it easier to run without this change (as well - so people can run the same CI command), making a run_doxygen.sh with body:

script_folder=`dirname $0`

$script_folder/run_diff.sh DOXYGEN "$@"

and the same instructions as run_lint.sh in the docs would work.

scripts/run_doxygen.sh

scripts/filter_expected_warnings.py

allredj

🚫
This PR failed Diffblue compatibility checks (cbmc commit: 70edaf4).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90096075
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 6b9af6d).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90098345

chrisr-diffblue · 2018-11-02T16:33:42Z

scripts/filter_expected_warnings.py

+        self.warnings_list = []
+        current = None
+
+        warning_start_expr = re.compile(r'[^:]+/([^:/]+):\d+: (warning.*$)')


A comment describing in prose, with example input/result for the regexp would be nice here... e.g. "Given an input line such as blah, this regexp will match and return foo in group(1), and wibble in group(2)

chrisr-diffblue · 2018-11-02T16:42:40Z

doc/architectural/compilation-and-development.md

+To build it locally, run `scripts/run_doxygen.sh`. (This script will filter out
+expected warning messages from doxygen, so that new problems are more obvious.)
+HTML output will be created in `doc/html/`. The index page is
+`doc/html/index.html`.


Since this PR introduces the reference file scripts/expected_doxygen_warnings.txt that will need to be maintained (e.g. removing expected warnings if they get fixed, or adding new expected warnings where we introduce stuff that is correct, but doxygen warns about due to its foibles) could you add a little more to the docs here to explain the appropriate actions?

chrisr-diffblue · 2018-11-02T16:44:03Z

scripts/filter_expected_warnings.py

+
+    # print unexpected warnings
+    unexpected_warning_found = new_warnings.print_warnings_not_in_other_list(
+        expected_warnings, '')


I'd be tempted to have the prefix here be '+' rather than '' so that it's consitent with, say 'diff' format, and also consistent with the 'missing warnings case'

thk123

In future can you not resolve my comments as I find it useful for tracking which ones have been applied (not an issue for a small PR like this of course!)

chrisr-diffblue · 2018-11-02T16:50:13Z

scripts/filter_expected_warnings.py

+
+    # print expected warnings which aren't found
+    expected_warning_not_found = expected_warnings.print_warnings_not_in_other_list(
+        new_warnings, '-')


One other thought.... In the case where the scripts reports a missing 'expected' warning (yay!) - how easy will it be to figure out which line in the list of expected warnings to delete? Is there anything that you can add to the diagnostic here (such as line number in the expected warnings file) that would make it easier to fix that file?

I know from experience that it's already very easy, and I'm sure you'll agree with me after you've fixed all those warnings and had a go yourself. 😃

OK, fine - I'm just not having a good mental picture of what the output of this script will look like :-) I'm happy to take your word for the easy of maintenance of the warnings file.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 0b20bc3).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90103410

Add a list of expected warnings from Doxygen, a Python script to filter out those warnings from the output from Doxygen, and a simple bash script to run doxygen using this filter. In some cases, it would be relatively easy to fix the Doxygen documentation so that we don't get these warnings. However, in other cases there are bugs in Doxygen where the code and documentation are perfectly valid, and Doxygen gives an erroneous warning message. Currently, CI uses another script to filter out warnings related to lines of code which have not been changed. However, this doesn't work well because the line number reported for a warning is often not the line where the problem lies. The new script should be useful for CI, and also for checking changes locally before running CI.

johnnonweiler · 2018-11-02T17:14:28Z

I've expanded on the documentation following the comment from Chris.

johnnonweiler requested review from forejtv, peterschrammel and thk123 as code owners October 30, 2018 12:11

owen-mc-diffblue approved these changes Oct 30, 2018

View reviewed changes

allredj reviewed Oct 30, 2018

View reviewed changes

hannes-steffenhagen-diffblue approved these changes Nov 1, 2018

View reviewed changes

scripts/filter_expected_warnings.py Outdated Show resolved Hide resolved

scripts/filter_expected_warnings.py Outdated Show resolved Hide resolved

scripts/filter_expected_warnings.py Outdated Show resolved Hide resolved

hannes-steffenhagen-diffblue added the work in progress label Nov 1, 2018

allredj reviewed Nov 1, 2018

View reviewed changes

hannes-steffenhagen-diffblue approved these changes Nov 1, 2018

View reviewed changes

scripts/filter_expected_warnings.py Outdated Show resolved Hide resolved

scripts/filter_expected_warnings.py Outdated Show resolved Hide resolved

allredj reviewed Nov 1, 2018

View reviewed changes

hannes-steffenhagen-diffblue reviewed Nov 1, 2018

View reviewed changes

scripts/filter_expected_warnings.py Outdated Show resolved Hide resolved

hannes-steffenhagen-diffblue reviewed Nov 1, 2018

View reviewed changes

scripts/filter_expected_warnings.py Outdated Show resolved Hide resolved

allredj reviewed Nov 1, 2018

View reviewed changes

allredj reviewed Nov 2, 2018

View reviewed changes

johnnonweiler mentioned this pull request Nov 2, 2018

Use new Doxygen script in Travis CI #3258

Merged

6 tasks

owen-mc-diffblue approved these changes Nov 2, 2018

View reviewed changes

thk123 suggested changes Nov 2, 2018

View reviewed changes

scripts/run_doxygen.sh Show resolved Hide resolved

scripts/filter_expected_warnings.py Show resolved Hide resolved

allredj reviewed Nov 2, 2018

View reviewed changes

johnnonweiler requested review from chrisr-diffblue, kroening, smowton and tautschnig as code owners November 2, 2018 16:13

thomasspriggs removed the work in progress label Nov 2, 2018

chrisr-diffblue reviewed Nov 2, 2018

View reviewed changes

thk123 approved these changes Nov 2, 2018

View reviewed changes

chrisr-diffblue reviewed Nov 2, 2018

View reviewed changes

allredj reviewed Nov 2, 2018

View reviewed changes

chrisr-diffblue approved these changes Nov 2, 2018

View reviewed changes

chrisr-diffblue merged commit b883685 into diffblue:develop Nov 5, 2018

johnnonweiler deleted the doxygen-warnings-filter branch November 5, 2018 13:01

Add script to filter Doxygen warnings #3250

Add script to filter Doxygen warnings #3250

Uh oh!

Conversation

johnnonweiler commented Oct 30, 2018

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

hannes-steffenhagen-diffblue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

allredj 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!

Uh oh!

Uh oh!

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

johnnonweiler commented Nov 2, 2018

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

johnnonweiler commented Nov 2, 2018

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

owen-mc-diffblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thk123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj 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!

thk123 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