Skip to content

Conversation

johnnonweiler
Copy link
Contributor

@johnnonweiler johnnonweiler commented Oct 8, 2018

Following on from #3082, this addresses some more low hanging fruit for reducing the number of warnings when running doxygen. Please see the individual commits for details.

  • Each commit message has a non-empty body, explaining why the change was made.
  • My contribution is formatted in line with CODING_STANDARD.md.
  • 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.

John Nonweiler added 5 commits October 5, 2018 11:35
Doxygen was giving a warning message about "multiple use of section
label 'deprecated'", so this commit changes the section label.  There
are no references to this section, so the label is only changed in
one place.  (I can't find any other uses of this section label, so
the warning message seems to be wrong, but it doesn't hurt to fix it.)
Delete documentation for parameters which no longer exist
Copy link
Contributor

@chrisr-diffblue chrisr-diffblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look basically sound, so I'm happy with those - but I note that CI is failing for the doxygen run, so that will need to be fixed before this is merged. I'm not immediately seeing why it is failing, so it may just be a travis glitch that is resolved with a build restart - but needs investigating.

@@ -90,7 +90,7 @@ is initially populated from `irep_ids.def`, so for example the fourth entry
in `irep_ids.def` is `“IREP_ID_ONE(type)”`, so the string “type” has index 3.
You can refer to this \ref irep_idt as `ID_type`. The other kind of line you
see is `“IREP_ID_TWO(C_source_location, #source_location)”`, which means the
\ref irep_idt for the string “#source_location” can be referred to as
\ref irep_idt for the string “\#source_location” can be referred to as
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this change doesn't actually achieve what it is supposed to do? Doxygen now appears to be complaining that the link request could not be satisfied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changed worked. The problem is the other #source_location on the previous line. (I'm not sure how I missed this earlier!)

This has turned out to be much more tricky to fix - adding a \ doesn't work in this case (as explained in https://stackoverflow.com/a/36750674 ). I haven't found a perfect solution to this, but have now committed something that works with slightly different double quotes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good, especially with the elaborate commit message. It's not beautiful, but anyone can easily figure out what's going on here.

Doxygen produced a warning about a reference it couldn't find, because
of a '#' symbol used in the comment (which the author didn't intend to
be a reference).  Usually, '#' could be escaped '\#', but that doesn't
work in this case, because it appears within quotes (as explained in
https://stackoverflow.com/a/36750674 ).  I couldn't find a way to get
this to work with the quotes that had been used, so I have changed it
to use different quotes.
@johnnonweiler
Copy link
Contributor Author

@tautschnig, @chrisr-diffblue please could you let me know if you're still happy with this, given my additional commit?

@chrisr-diffblue
Copy link
Contributor

It's a bit ugly, but it does look like this is the only workaround to what seems like a weakness in Doxygen. As such, I'm OK with the additional tweak. I'm agnostic on whether this additional commit should be squashed into the earlier commit or not.

@johnnonweiler
Copy link
Contributor Author

I'd prefer not to squash it, as it is actually completely independent from the change in the previous commit. (Travis is supposed to only complain about doxygen issues on lines which have been changed, but in this case doxygen was reporting the wrong line number in its warning message.)

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR failed Diffblue compatibility checks (cbmc commit: eea5403).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/87214638
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).

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: eea5403).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/87214638

Copy link
Contributor

@NathanJPhillips NathanJPhillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other mistakes on the lines you have fixed things on, but all your changes are an improvement so I approve.

@tautschnig tautschnig merged commit d63fb7a into diffblue:develop Oct 9, 2018
@johnnonweiler johnnonweiler deleted the doc/reduce-doxygen-warnings-2 branch October 9, 2018 10:13
This was referenced Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants