DOC: Fix unbalanced grouping commands Doxygen warnings

[jhlegarreta]

Fix unbalanced grouping commands Doxygen warnings.

Fixes:

Modules/Core/Common/include/itkAutoPointer.h:261: warning: unbalanced grouping commands

warnings across classes.

The warning is raised when the
Utilities/Doxygen/itkdoxygen.pl

Doxygen Perl script processes the files at issue and starts a /**@{ block that gets interrupted by an unexpected symbol (e.g. a brace) without being previously closed with the corresponding /**@}*/ ending token.

Raised for example in:
https://open.cdash.org/viewBuildError.php?type=1&buildid=8344134

PR Checklist

• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)

[jhlegarreta]

I checked the /**@{ /**@}*/ blocks for every file manually following #3654 (comment).

However, clang-format already warned me locally that it did not like the changes: it does not like changes in 61 files out of 79.

The pattern that we require to solve the Doxygen issues (that clang-format dislikes) looks to be "we require a blank line before closing the class with a };", as if we were to fulfill the condition in this comment:

ITK/Utilities/Doxygen/itkgroup.pl

Line 2 in 047fa69

# at next empty line add a //@}

@hjmjohnson @thewtex @dzenanz any suggestion on how we could work around this without having to recur to turning it on/off on each of these lines?

The other hypothesis is that the variable values in this condition

ITK/Utilities/Doxygen/itkgroup.pl

Line 68 in 047fa69

if($endparencount > 1 && ($semicount > 1 || $endbracecount > 1))

or the condition itself should be modified. Maybe the "//@}\n\n" ending block should be inserted at buffer_lines - 1 if we cannot work around the clang-format issue, and if that is possible.

Another possibility would be to submit only the changes that make clang-format happy, and increasingly add the rest on a case-by-case in case the required blank line can be introduced at some place other than the one where I introduced it.

[dzenanz]

It feels like we should update clang format style to allow the changes proposed here. I am not sure whether that can accomplish it.

[dzenanz]

I am running the build script with doxygen updated to 1.9.5, so we don't have to wait until tomorrow to see the updated output.

[jhlegarreta]

It feels like we should update clang format style to allow the changes proposed here. I am not sure whether that can accomplish it.

Looks like there may be a way:
https://stackoverflow.com/questions/33547172/clang-format-empty-line-between-end-of-class-declaration-and-closing-of-a-names
https://stackoverflow.com/questions/70732683/how-to-add-blank-lines-between-definitions

[dzenanz]

The second suggestion would require us to move to a newer clang-format (14 or later). We are currently at 8.0.0. Moving to a newer one shouldn't be a big change. The first suggestion looks like it would work now. Can you try it?

[dzenanz]

Doxygen 1.9.5 produces 100+ new warnings of style: Generating docDocumentation/Doxygen/NeighborhoodIterators.dox:96: warning: Unexpected token TK_HTMLTAG found as part of a title section. All warnings here. @jhlegarreta should we keep this version?

[jhlegarreta]

Doxygen 1.9.5 produces 100+ new warnings of style: Generating docDocumentation/Doxygen/NeighborhoodIterators.dox:96: warning: Unexpected token TK_HTMLTAG found as part of a title section. All warnings here. @jhlegarreta should we keep this version?

😓 I would keep the old version for now then. I would upgrade to the newer version once we have managed to fix the current warnings (which are still present in the new version).

[dzenanz]

I just reverted doxygen to 1.8.16.

[jhlegarreta]

The first suggestion looks like it would work now. Can you try it?

Looks like our clang-format options prevent it from working as described in the thread: https://github.com/InsightSoftwareConsortium/ITK/actions/runs/3727206860/jobs/6321140027#step:4:86

@hjmjohnson Do you happen to know the clang-format option that should do the work for our current version?

[dzenanz]

Adding end of class comment does trigger the described workaround:

  /** Sets the value of the image buffer at the current index value to the
   * specified value.  */
  void
  SetPixelValue(InternalPixelType * const imageBufferPointer, const PixelType & pixelValue) const noexcept
  {
    m_NeighborhoodAccessor.Set(imageBufferPointer + m_PixelIndexValue, pixelValue);
  }

}; // end of class

This is an excerpt from ITK\Modules\Core\Common\include\itkBufferedImageNeighborhoodPixelAccessPolicy.h, near the end.

[jhlegarreta]

#3805 (comment) OK, I was thinking about the namespace ending. I can do this, but I do not think we are willing to have this as a long-term solution.

@thewtex @blowekamp @hjmjohnson any thoughts?

[thewtex]

Another approach could replace itkdoxygen.pl with a Python script 🐍

[jhlegarreta]

Another approach could replace itkdoxygen.pl with a Python script 🐍

The problem is rather clang-format disliking the insertion of a blank line before the end of class closing bracket line without the latter having an inline comment.

[thewtex]

The problem is rather clang-format disliking the insertion of a blank line

Can we remove the insertion of the blank line in the Python script migration?

[jhlegarreta]

Can we remove the insertion of the blank line in the Python script migration?

It should rather be introduced: clang-format dislikes it and it keeps trying to remove it.

I do not have the bandwidth to translate the script into Python.

Alternatives:

• If you feel like modifying the Perl script to add such a blank line and have this tested, please go ahead ▶️.
• If you feel like to translating the script to Python and introducing the blank line, please go ahead ▶️.
• Otherwise, pinging @hjmjohnson to see if he can allow the blank like without changing the script.

[jhlegarreta]

Do you think the below approach plugged right after

ITK/Utilities/Doxygen/itkgroup.pl

Line 67 in 23da3df

}

can be made to work?

if($line =~ /\};/ )
{
  // Take the current buffer $buffer (assuming it includes the current line `};` as well) and add a blank line
  // immediately before `};` (if the current buffer does not include the current line, then include it right after
  // the buffer end)

  (doTheAboveStuff)

  // (I think we do not care that much if }; is not the end of the class definition (i.e. even if a lot of blank lines
  // are inserted) as this is only to produce proper Doxygen code)

  (save the $buffer)
}

so that we can get the result in #3805 (comment) without needing to add // end of class and thus bypass clang's unhappyness about adding a blank line? Or maybe there is another way to achieve it

@thewtex @tbirdso maybe one of you is skilled at Perl to give that a try? Thanks.

[dzenanz]

I still think that moving to latest stable clang-format (currently 15.0.7) is the best solution.

[jhlegarreta]

I still think that moving to latest stable clang-format (currently 15.0.7) is the best solution.

Would that fix the issue here ? Isn't the config itself, rather than the clang-format version, what might help ?

[dzenanz]

The current version ignores (does not support) the config setting we need. As per this SO answer, we need SeparateDefinitionBlocks, which was introduced with clang 14. So we need version 14+.

We already have MaxEmptyLinesToKeep: 2 in our config, and that is not enough.

[jhlegarreta]

#3805 (comment) OK, then +100 for upgrading. @hjmjohnson ?

[hjmjohnson]

@jhlegarreta I trust the judgment of those who have invested time into this PR. I will not have time to review it carefully.

[jhlegarreta]

Cross-referencing InsightSoftwareConsortium/ITKClangFormatLinterAction#3.

[dzenanz]

Rebased

[dzenanz]

This does reduce number of warning: unbalanced grouping commands from 92 down to 20.

[jhlegarreta]

So I have revisited the comments here and the related issue (#3654):

• We did not make DOC: Fix unbalanced grouping commands Doxygen warnings #3805 (comment) happen when the Perl script was translated to Python in PR Convert itkgroup Doxygen script to Python. #5157: if I read correctly,
  a5db4a8#diff-41905fce6b73c41cc93104323e7d01cfec55393204c0069acc931745fa8438aeL56-L66

  https://github.com/InsightSoftwareConsortium/ITK/pull/5157/files#diff-56c509b541d6d68af2472e42febc63da9ec3e63b1d90bffd64a3e7bac3b975feR63-R68

  it was not done, unless the regular expression hits are different.

• The other possibility was transitioning to a newer clang (DOC: Fix unbalanced grouping commands Doxygen warnings #3805 (comment)). PR Update to clang format 19 support #5015 transitioned to clang 19, and
  https://github.com/InsightSoftwareConsortium/ITK/pull/5015/files#diff-1026e0038b722990204a42bed8a6f7c0ec2302aa79e3fad1959d62ba968edfa2R239, thus, using Leave (not introducing any new blank line, as opposed to Always cf. the stackoverflow answer posted in the link of that comment). So that looks OK to me, as now clang does not complain.

As for the discussion in issue #3654:

• The location of the *@} ending markers has not been changed by the Python script as far as I understand, so the underlying issue identified by @albert-github (Doxygen "unbalanced grouping commands" warnings #3654 (comment)) seems not to have been addressed. According to Doxygen "unbalanced grouping commands" warnings #3654 (comment) and Doxygen "unbalanced grouping commands" warnings #3654 (comment), the purpose of the Python script should be reached by using other Doxygen means (disclaimer: I have not read the Python script except for the above part).
• As for the comment Yes. We are currently using 1.9.4 and the latest release of Doxygen is 1.13.2. Time is likely better spent getting a newer version of Doxygen to work than maintaining the old one. in Doxygen "unbalanced grouping commands" warnings #3654 (comment), updating the Doxygen version in blaster (which is the one that reports to CDash, not the version in InsightSoftwareConsortium/ITKDoxygen@37cfce3#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R18, which is 1.9.3) has not helped in removing the warnings.
• The remaining 20 warnings, are they because the files were not changed in this PR or because it is a demonstration that the underlying issue is not a simple addition of whitespaces?

So, these are my thoughts:

1. We can mark this as "Ready for review", ignore that the underlying issue has not been addressed (the use of the now Python script to do the job that the Doxygen ALIASES should do according to @albert-github), merge this PR, close issue Doxygen "unbalanced grouping commands" warnings #3654, and open a new issue to address the Python script vs. Doxygen ALIASES use. This would introduce a supposedly unnecessary change into the code, and one that does potentially not address the underlying issue (cf. the comment about the remaining 20 warnings).
2. We can wait until somebody has the bandwidth to address the Python script vs. Doxygen ALIASES use issue, at which point we would see whether it does the trick and could eventually close this PR and issue Doxygen "unbalanced grouping commands" warnings #3654.

My vote is for 1 for the sake of our mental well-being (which includes seeing less noise in the dashboard). But I am happy if others have the bandwidth to look into the ALIASES solution.

[dzenanz]

I updated Doxygen on blaster to latest (1.13.2). Merging this PR sounds like a low-risk step in the right direction. Let's give people a few days to review (or maybe tackle option 2).

[albert-github]

With the ALIASES solution from me you mean my proposal in #4161 (comment) ?

Still I see more in that approach than by means of using a script to convert the code based on some special white space in the code (as this probably will break again in the future / miss some special cases.). The note I made in #4161:

Note: I think that running doxygen with -d preprocessor once with and once without the filter might help to identify where currently @{ ... @} is inserted so these files can be inspected (will be quite a bit of work though).

is probably still quite valid as it will be quite a bit of work in the already over stretched workloads.

[blowekamp]

I am not in favor of this commit. Adding "magic" white space lines to avoid warnings is not a manageable and robust solution.

The purpose to migrating the script to Python was that no one know Perl well enough to make the fix in the script. Now what the "grouping" script is in python it should be debuggable and fixable by the community.

[dzenanz]

Doxygen warning log is swamped by unbalanced grouping commands. This PR will reduce that noticeably, and I think it would be advantageous to have that while waiting for someone to get around to fixing the grouping script. I agree that this is not maintainable in the long term.

[jhlegarreta]

Re: #3805 (comment)

With the ALIASES solution from me you mean my proposal in #4161 (comment) ?

Yes.

Still I see more in that approach than by means of using a script to convert the code based on some special white space in the code (as this probably will break again in the future / miss some special cases.). The note I made in #4161:

Note: I think that running doxygen with -d preprocessor once with and once without the filter might help to identify where currently @{ ... @} is inserted so these files can be inspected (will be quite a bit of work though).

is probably still quite valid as it will be quite a bit of work in the already over stretched workloads.

I am open to any solution that is more robust than this PR. I think I made this clear. I do not have the bandwidth to try all avenues.

Re: #3805 (comment)

I am not in favor of this commit. Adding "magic" white space lines to avoid warnings is not a manageable and robust solution.

The purpose to migrating the script to Python was that no one know Perl well enough to make the fix in the script. Now what the "grouping" script is in python it should be debuggable and fixable by the community.

There was a guess in #3805 (comment). Feel free to try.

[albert-github]

I saw that in the output e.g. the mainpage was empty and its information should have been retrieved from the MainPage.dox file.
It looks like the problem also is a result from the filtering and as the dox files don't contain "C++" code but just comments, there is no need to run them through the "grouping filter".

Proposed patch: diff.patch

[albert-github]

At all, in #4161 there were 2 suggestions to bypass the problems of the "grouping" script

1. Through ALIASES: \itkGroupStart / \itkGroupEnd
2. Through \{ / \}

subsequently (no difference for doxygen) the used command identifier:
a. commands start with \
b. commands start with @

What is the consensus to use?

[dzenanz]

I saw that in the output e.g. the mainpage was empty and its information should have been retrieved from the MainPage.dox file. It looks like the problem also is a result from the filtering and as the dox files don't contain "C++" code but just comments, there is no need to run them through the "grouping filter".

Proposed patch: diff.patch

I created PR #5225 with the proposed changes.

[dzenanz]

Option 2 (\{ and \}) seems too cryptic to me. I prefer the alias.

As backslash gets so much use already in C++, CMake, Python and elsewhere, maybe @ is better?