Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation: WARN_NO_PARAMDOC not disabled by EXTRACT_ALL? #8905

Closed
traud opened this issue Nov 22, 2021 · 10 comments
Closed

Documentation: WARN_NO_PARAMDOC not disabled by EXTRACT_ALL? #8905

traud opened this issue Nov 22, 2021 · 10 comments
Labels

Comments

@traud
Copy link

traud commented Nov 22, 2021

The online documentation (and the configuration file generated on default with $ doxygen -g) state for WARN_NO_PARAMDOC: ‘If EXTRACT_ALL is set to YES then this flag will automatically be disabled.’

Example Doxyfile A:

WARN_AS_ERROR    = YES
WARN_NO_PARAMDOC = YES
EXTRACT_ALL      = YES

Example Doxyfile B:

WARN_AS_ERROR    = YES
WARN_NO_PARAMDOC = NO
EXTRACT_ALL      = YES

Consequently, those two example configurations should produce the same result. However, when I use the following example C code, only the one with WARN_NO_PARAMDOC enabled errors.

#include <stdio.h>

/*!
 * \brief Hello World!
 * \return void
 */
void hello() {
	puts("Hello World!");
}

/*!
 * \brief main
 * \retval 0 Always
 */
int main(void) {
	hello();
	return 0;
}

Personally, I like the behavior to be able to have more fine grained control. Therefore, I file this as a miss in the Doxygen configuration file documentation. However, if I misunderstood something, please, correct me!

Version
1.9.1 on Ubuntu 21.10, re-tested with current binary release (1.9.2)

@albert-github
Copy link
Collaborator

There is now a discrepancy between the documentation and the implementation.

A bisect reveals that the commit:

2428c5ad6d971793a2942a0cf08a4b2449a98551 is the first bad commit
Date:   Mon Jan 21 13:51:08 2019 +0100

    Warning for documented return of void type function

    In case a void type function is documented it is not allowed to used a `\return` or `\retval` command. The test was present but not correct
    - test on `g_hasReturnCommand ` which signals whether of not `\ret...` is used instead of  `g_memberDef->hasDocumentedReturnType()`
    - removed test on emty return type (empty return type is assumed to be non void, see e.g. C, Fortran).
    - order of the tests is important

causes this extra warning. The waning as is is correct and caused by \return void but in this case should not be present.
(the warning is introduced in 1.8.16).

albert-github added a commit to albert-github/doxygen that referenced this issue Nov 22, 2021
…TRACT_ALL?

Placing guard around the warning.
Making it consistent with the case that when a return type is needed.
@albert-github
Copy link
Collaborator

I've just pushed a proposed patch, pull request #8906

@traud
Copy link
Author

traud commented Nov 23, 2021

Can you elaborate a bit more on the proposed change? If I am not misreading, with that change, EXTRACT_ALL must be NO to get that warning about void-return in the future. Is that correct? Mhm. Then, I have a real problem: The project I am about had around 2,500 warnings. All, I repeat, all of them were valid. Right now, I am at zero. However, with EXTRACT_ALL = YES (and EXTRACT_STATIC = YES), after disabling those, I get around 25,000 warnings. Yes, OK, because I am clever and know the inner stuff of Doxygen, I am able to pick just those related to void-return, and fix them. However, EXTRACT_ALL = NO is no use case in that project (or I am misusing Doxygen). Consequently, there is no reason to fix those, actually.

My plan was/is: Enable Doxygen to be a syntax checker to find the wildest errors in that project. In that project, Doxygen is not even used to create documentation. Because of that, I were looking for zero warnings, so developers at least look at their newly introduced issues and possible fix them theirselves. Most of those were simply typos and/or internal changes over time. Minor nits. However, no reason to confuse fellow developers.

Furthermore, my fear, I would have never ever looked at EXTRACT_ALL = NO because of that zillion of warnings. Consequently, I would not have known about this return thing. Well, yes, even the latest compiler Clang is warning about this when its warning -Wdocumentation is enabled. Anyway, is there a way for a middle ground? I fixed around 750 warnings related to this void-return warning. Are you telling me that was useless (in my case), and I should have reported this upstream earlier?

What about moving the warning about void-return into WARN_IF_DOC_ERROR, so the state of EXTRACT_ALL does not matter?

In other words, I would prefer to keep this issue rather than fixing it that way. Does my vote count? Or is the latter a thumbs up on enhancement #8073? Perhaps, really, I am misusing Doxygen. Again, I would like to utilize Doxygen just like a syntax checker.

@albert-github
Copy link
Collaborator

My personal opinion:

Disabling and enabling warning is a tricky business also for different users / usage and this issue underlines it again.

The documentation states for WARN_IF_DOC_ERROR:

If EXTRACT_ALL is set to YES then this flag will automatically be disabled.

and I think the proposed change just does this and makes it also consistent with other return messages etc,.

Like you wrote:

My plan was/is: Enable Doxygen to be a syntax checker to find the wildest errors in that project.

the intention of doxygen is not to do syntax checking on the code, doxygen expects valid code and from that tries to generate some documentation. When the code is not valid the behavior is undefined. So from that you are, in my opinion, indeed misusing doxygen.
You also wrote:

Most of those were simply typos and/or internal changes over time. Minor nits.

so actually errors in the documentation of the code, this is one of the most annoying things for, new, developers and they should be fixed. It also never hurts to run a separate spelling checker on the documentation.

The fixing of the void-return is not useless as doxygen thinks this is a bug (a void function does not return anything. This is made even more clear in e.g. Fortran where there are functions and subroutines).

It might be a thumbs up for #8073 but maybe also about redesigning the entire warnings (I once thought about giving each warning, type, its own warning number,so users can select their own waning types, but this will be a bigggg job).

@traud
Copy link
Author

traud commented Nov 24, 2021

I think we have a misunderstanding. I am not using Doxygen to find faults in the underlying C source code. I am about using Doxygen as syntax checker for its the Doxygen commands added to the source code. Doxygen is quite good in that thanks to WARN_IF_DOC_ERROR plus INTERNAL_DOCS. However, I am not sure which interesting other warnings I miss.

The documentation states for WARN_IF_DOC_ERROR:

Wuup. Where is that? The online documentation and the default generation configuration file created via $ doxygen -g gives that just for WARN_NO_PARAMDOC (and WARN_IF_UNDOCUMENTED). Therefore, I wonder if it is not possble to move this void-return-check from WARN_NO_PARAMDOC into WARN_IF_DOC_ERROR. Semantically, it looked like the correct place, at least for me.

@albert-github
Copy link
Collaborator

albert-github commented Nov 27, 2021

  1. misunderstanding, fortunately doxygen is not used by you to check the underlying code but to check the documentation. Sorry for the miscommunication.
  2. I don't know why EXTRACT_ALL entered here in the solution (Ahh I see it was mentioned in the title). I have to rethink the solution.

albert-github added a commit to albert-github/doxygen that referenced this issue Nov 27, 2021
…TRACT_ALL?

Solution had to be independent of the setting of `WARN_NO_PARAMDOC` in this routine.
@albert-github
Copy link
Collaborator

I just updated the pull request (@traud please test).

@traud
Copy link
Author

traud commented Nov 27, 2021

Yes, my issue report was against the documentation, otherwise I would have not noticed it actually, therefore EXTRACT_ALL came into play. Your change looks good to me, although the final change introduces a new whitespace – not sure if that was intended. Anyway, I might not be able to test it because I cannot build Doxygen from source (yet). Perhaps I find a time slot in the next week to go for that. Thanks for replicating my issue, taking over, and that Pull Request! Much appreciated.

doxygen added a commit that referenced this issue Nov 29, 2021
issue #8905 Documentation: WARN_NO_PARAMDOC not disabled by EXTRACT_ALL?
@albert-github albert-github added the fixed but not released Bug is fixed in github, but still needs to make its way to an official release label Nov 30, 2021
@albert-github
Copy link
Collaborator

Code has been integrated in master on GitHub (please don't close the issue as this will be done at the moment of an official release).

@doxygen
Copy link
Owner

doxygen commented Dec 31, 2021

This issue was previously marked 'fixed but not released',
which means it should be fixed in doxygen version 1.9.3.
Please verify if this is indeed the case. Reopen the
issue if you think it is not fixed and please include any additional information
that you think can be relevant (preferably in the form of a self-contained example).

@doxygen doxygen removed the fixed but not released Bug is fixed in github, but still needs to make its way to an official release label Dec 31, 2021
@doxygen doxygen closed this as completed Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants