Doxygen: Extra added when a node contains multiple \remarks, \warning, etc.

marzer commented 6 years ago

See title. Essentially, this:

/// \brief  ...
/// \remarks    Some noteworthy remark.
/// \attention Something kinda critical.

Now has an extra   inserted in between the two blocks, causing the pages to look overly spaced out if this is a common pattern (as it is in one of our documents). Can't say for sure when the behavior changed, but it would have been in one of the last 9 commits, since that was how many I was behind when I last pulled.

(love the project, btw!)

mosra commented 6 years ago

Huh. If this was really introduced in the last 9 commits and none of the tests caught it, then I should fire myself :D Will look into it and report back.

Maybe related to this -- I know there are superfluous   elements inserted when using recursive @copybrief and similar commands (so e.g. a @copybrief that copies contents of another @copybrief). That's a Doxygen bug and I yet have to report it / fix it upstream.

marzer commented 6 years ago

Hmmn, OK, so some more on this. I've double-checked the cause, and I wasn't specific enough in my initial report. I've realized it's only when there's an explicit whitespace-only line in the comment block. Some examples:

Correct case:

/// \brief Testy McTestface
/// \attention OH NOES
/// \remarks A remark
/// \remarks Another remark

Adding a whitespace line between unrelated blocks:

/// \brief Testy McTestface
/// \attention OH NOES
///            <---- (spaces)
/// \remarks A remark
/// \remarks Another remark

Adding a whitespace line between two blocks of the same type:

/// \brief Testy McTestface
/// \attention OH NOES
/// \remarks A remark
///            <---- (spaces)
/// \remarks Another remark

(these used to be merged together, regardless of the whitespace line?)

The 'whitespace' line in my examples is borne out of an annoying side-effect of some refactoring, and appears in a lot of places in our codebase. If I explicitly trim it down to /// in each case the problem is resolved, though it is curious that it was handled more generously before?

mosra commented 6 years ago

Hi, sorry that I didn't get to this yet, work piled up.

If you got some time, could you paste the relevant parts of the Doxygen XML output files here? To me this looks like some unfortunate Doxygen parsing glitch that depends on trailing whitespace ... and m.css is only picking up whatever it generated. Also, are you sure it didn't happen before? I don't remember touching anything that would affect it. Could you bisect to a particular commit where this started to appear?

mosra commented 6 years ago

Hum, sorry, can't reproduce. This is how my test looks like (whitespace highlighted):

This is the generated XML output for the foo() function (apart from crazy broken indentation and spacing, seeing no extra paragraphs there):

        <detaileddescription>
<para><simplesect kind="attention"><para>OH NOES</para></simplesect>
<simplesect kind="remark"><para>Remark </para></simplesect>
<simplesect kind="remark"><para>Another remark </para></simplesect>
</para>        </detaileddescription>

And this is the final HTML output:

<p>Testy McTestface.</p>
<aside class="m-note m-warning"><h4>Attention</h4><p>OH NOES</p></aside><aside class="m-note m-default"><h4>Remark</h4><p>Remark</p><p>Another remark</p></aside>

Tried with both stock Doxygen 1.8.14 and my random Git fork of it.

Anything I missed here? Some non-default Doxygen config? Windows line endings? Some additional patches to m.css you have there?

marzer commented 6 years ago

I'm definitely sure it didn't happen before, since I've been very immersed in documentation over the last month. Don't know that I'll be able to narrow it down to a specific commit for you but I can definitely give you some doxy XML. Here's a real example from the codebase:

/// \brief  Should this task be batched together with other matching tasks, where possible?
/// 
/// \remarks If this is non-zero, the renderer groups the task together with other tasks with a matching
///         BatchID and draws them in batched mode, so that it can submit them all with one Draw
///         call to the underlying graphics API.
///      <---- (a tab character)
/// \remarks The BatchID should be the same for all RenderTasks setting the same state in Bind.
///         The only thing that should differ between them is the BatchKey. Recommended way of
///         generating this key is to use a hash of the memory addresses of all the bindables
///         (buffers, resources) you set during Bind (See Material.cpp for an example of this).
/// 
/// \remarks Only applies to geometry layers.

Generates this:

<memberdef kind="variable" id="struct_os_engine_1_1_render_task_1a0576cca1e4358f4632cd5e07d4d0e3c0" prot="public" static="no" mutable="no">
  <type>uint64_t</type>
  <definition>uint64_t OsEngine::RenderTask::BatchID</definition>
  <argsstring />
  <name>BatchID</name>
  <initializer>= 0</initializer>
  <briefdescription>
    <para>Should this task be batched together with other matching tasks, where possible?</para>
  </briefdescription>
  <detaileddescription>
    <para>
      <simplesect kind="remark">
        <para>If this is non-zero, the renderer groups the task together with other tasks with a matching BatchID and draws them in batched mode, so that it can submit them all with one Draw call to the underlying graphics API.</para>
      </simplesect>
      <linebreak />
      <simplesect kind="remark">
        <para>The BatchID should be the same for all RenderTasks setting the same state in Bind. The only thing that should differ between them is the BatchKey. Recommended way of generating this key is to use a hash of the memory addresses of all the bindables (buffers, resources) you set during Bind (See Material.cpp for an example of this).</para>
      </simplesect>
      <simplesect kind="remark">
        <para>Only applies to geometry layers.</para>
      </simplesect>
    </para>
  </detaileddescription>
  <inbodydescription />
  <location file="D:/Repositories/OsEngine/include/OsEngine/IRenderable.h" line="365" column="1" bodyfile="D:/Repositories/OsEngine/include/OsEngine/IRenderable.h" bodystart="365" bodyend="-1" />
</memberdef>

Producing this:

So it does look like I've triggered a bug in doxygen, since there's extra tags in the XML output. Don't know why it hasn't occurred until recently. Is a workaround of filtering out extraneous <linebreak/> tags between consecutive <simplesect> blocks a reasonable idea?

marzer commented 6 years ago

RE things you might have missed: I tried with both CRLF and LF line endings, neither made a difference. I tried the current stable release version of doxygen and a recent fork of it (~4 days ago) m.css is unmodified AFAIK my doxygen config is standard, I haven't changed anything significant beyond the project paths and some options related to preprocessor #defines.

O_o

marzer commented 6 years ago

Since figuring out that whitespace was the culprit it's easy enough to do a regex search-and-replace over the codebase to fix all occurrences, so I guess it's not really much of an issue, if you want to consider it closed/not a bug.

mosra commented 6 years ago

I just copied the above snippet verbatim, removed <---- (a tab character) from it so there's exactly ///<tab><tab><space> on that line, nope, couldn't reproduce. Tried different combinations of tabs and spaces, even indenting the whole thing, no change.

My XML output, again pasted verbatim including the original awful indentation:

<para>Should this task be batched together with other matching tasks, where possible? </para>        </briefdescription>
        <detaileddescription>
<para><simplesect kind="remark"><para>If this is non-zero, the renderer groups the task together with other tasks with a matching BatchID and draws them in batched mode, so that it can submit them all with one Draw call to the underlying graphics API.</para></simplesect>
<simplesect kind="remark"><para>The BatchID should be the same for all RenderTasks setting the same state in Bind. The only thing that should differ between them is the BatchKey. Recommended way of generating this key is to use a hash of the memory addresses of all the bindables (buffers, resources) you set during Bind (See Material.cpp for an example of this).</para></simplesect>
<simplesect kind="remark"><para>Only applies to geometry layers. </para></simplesect>
</para>        </detaileddescription>

More ideas:

My XML output has this ugly mis-indentation, yours doesn't. Did you hand-format it for clarity or you have some different Doxygen edition that doesn't produce such awful mess?
I'm on Linux. Could this really be a Windows-specific thing (oh well)? Do you have a possibility to test on another system?

Is a workaround of filtering out extraneous tags between consecutive blocks a reasonable idea?

Yes, that's completely reasonable (I'm doing that for a shitload of other things anyway, so one more workaround won't hurt). But since I can't reproduce, I can't implement that because I have nothing to test against... :/

marzer commented 6 years ago

The original XML was a mess, I re-formatted the XML using for the sake of readability (using https://www.freeformatter.com/xml-formatter.html). I checked the output before and after to ensure it was structurally the same, though if you're curious the original version of my comment on this thread was the ugly version so you should be able to see it in the revision history.

Could definitely be a windows-specific thing; at this stage it's looking like the only thing separating our two test environments. I'm not really in a position to test on Linux any time soon, I'm afraid; none of the machines I work with at work or home run linux so I'd be installing it explicitly to test this. Not ruling it out but it would at least be a week or so before I could do it.

mosra commented 6 years ago

I also triple-checked that my editor didn't remove the trailing whitespace on save. Tried with CR+LF on Linux, also no change.

This is a very exciting bug, heh :)

mosra commented 6 years ago

@marzer if you got some time to spare, I just pushed a branch named trailing-whitespace with d22228de1175bd9ec638e803ef1a136a2ae9409b, where I'm trying to reproduce your case. Could you run it on your machine like this?

cd m.css/doxygen
python -m unittest test.test_contents.BlocksTrailingWhitespace

It passes for me locally and also on the CI (Linux as well). If it works also on your machine, then the problem must be caused by something else; if it doesn't, then I'm afraid Doxygen on Windows has a bug.

marzer commented 6 years ago

Sure thing:

======================================================================
ERROR: test (test.test_contents.BlocksTrailingWhitespace)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\Repositories\m.css\doxygen\test\test_contents.py", line 64, in test
    self.assertEqual(*self.actual_expected_contents('File_8h.html'))
  File "D:\Repositories\m.css\doxygen\test\__init__.py", line 56, in actual_expected_contents
    with open(os.path.join(self.path, 'html', actual)) as f:
FileNotFoundError: [Errno 2] No such file or directory: 'D:\\Repositories\\m.css\\doxygen\\test\\contents_blocks_trailing_whitespace\\html\\File_8h.html'

----------------------------------------------------------------------
Ran 1 test in 0.234s

FAILED (errors=1)

mosra commented 6 years ago

Argh, platform-dependent Doxygen defaults :boom: :skull: :gun: ...

Can you try again with 24ff250e040db20fbc0ef20ecde2788f6e75a48c? Thank you and sorry for mess.

marzer commented 6 years ago

FAIL: test (test.test_contents.BlocksTrailingWhitespace)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\Repositories\m.css\doxygen\test\test_contents.py", line 64, in test
    self.assertEqual(*self.actual_expected_contents('File_8h.html'))
AssertionError: '<!DO[2408 chars]</p></aside><p><br /></p><aside class="m-note [528 chars]tml>' != '<!DO[2408 chars]</p><p>The BatchID should be the same for all [460 chars]tml>'
  <!DOCTYPE html>
  <html lang="en">
  <head>
    <meta charset="UTF-8" />
    <title>File.h file | My Project</title>
    <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:400,400i,600,600i%7CSource+Code+Pro:400,400i,600" />
    <link rel="stylesheet" href="m-dark+doxygen.compiled.css" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
  </head>
  <body>
  <header><nav id="navigation">
    <div class="m-container">
      <div class="m-row">
        <a href="index.html" id="m-navbar-brand" class="m-col-t-8 m-col-m-none m-left-m">My Project</a>
      </div>
    </div>
  </nav></header>
  <main><article>
    <div class="m-container m-container-inflatable">
      <div class="m-row">
        <div class="m-col-l-10 m-push-l-1">
          <h1>
            File.h <span class="m-thin">file</span>
          </h1>
          <p>A file.</p>
          <div class="m-block m-default">
            <h3>Contents</h3>
            <ul>
              <li>
                Reference
                <ul>
                  <li><a href="#func-members">Functions</a></li>
                </ul>
              </li>
            </ul>
          </div>
          <section id="func-members">
            <h2><a href="#func-members">Functions</a></h2>
            <dl class="m-dox">
              <dt>
                <span class="m-dox-wrap-bumper">void <a href="#ac07863d69ae41a4e395b31f73b35fbcd" class="m-dox">foo</a>(</span><span class="m-dox-wrap">)</span>
              </dt>
              <dd>Should this task be batched together with other matching tasks, where possible?</dd>
            </dl>
          </section>
          <section>
            <h2>Function documentation</h2>
            <section class="m-dox-details" id="ac07863d69ae41a4e395b31f73b35fbcd"><div>
              <h3>
                <span class="m-dox-wrap-bumper">void </span><span class="m-dox-wrap"><span class="m-dox-wrap-bumper"><a href="#ac07863d69ae41a4e395b31f73b35fbcd" class="m-dox-self">foo</a>(</span><span class="m-dox-wrap">)</span></span>
              </h3>
              <p>Should this task be batched together with other matching tasks, where possible?</p>
- <aside class="m-note m-default"><h4>Remark</h4><p>If this is non-zero, the renderer groups the task together with other tasks with a matching BatchID and draws them in batched mode, so that it can submit them all with one Draw call to the underlying graphics API.</p></aside><p><br /></p><aside class="m-note m-default"><h4>Remark</h4><p>The BatchID should be the same for all RenderTasks setting the same state in Bind. The only thing that should differ between them is the BatchKey. Recommended way of generating this key is to use a hash of the memory addresses of all the bindables (buffers, resources) you set during Bind (See Material.cpp for an example of this).</p><p>Only applies to geometry layers.</p></aside>
?                                                                                                                                                                                                                                                                             --------------------------------------------------------------------
+ <aside class="m-note m-default"><h4>Remark</h4><p>If this is non-zero, the renderer groups the task together with other tasks with a matching BatchID and draws them in batched mode, so that it can submit them all with one Draw call to the underlying graphics API.</p><p>The BatchID should be the same for all RenderTasks setting the same state in Bind. The only thing that should differ between them is the BatchKey. Recommended way of generating this key is to use a hash of the memory addresses of all the bindables (buffers, resources) you set during Bind (See Material.cpp for an example of this).</p><p>Only applies to geometry layers.</p></aside>
            </div></section>
          </section>
        </div>
      </div>
    </div>
  </article></main>
  </body>
  </html>

----------------------------------------------------------------------
Ran 1 test in 0.222s

FAILED (failures=1)

mosra commented 6 years ago

Okay, then it's clear this is a Windows-specific thing. I'll create a workaround based on the XML alone, hopefully I'll be able to cover all nasty corner cases there :)

mosra commented 6 years ago

Hmm. The more I think about this, the more I see that this should be reported to / fixed in Doxygen itself... turns out the workaround in m.css would affect places that are quite fragile already and I fear piling more code there would make it a maintenance nightmare, sorry :/

However, as far as I'm aware, Doxygen is migrating its bug tracker to GitHub right now so it's probably a good idea to wait a bit before things settle down there.

marzer commented 6 years ago

No worries, thanks for trying ^_^

marzer commented 5 years ago

@mosra good news: doxygen 1.8.15 fixes this issue shit news: doxygen 1.8.15 breaks a bunch of other things (╯°□°）╯︵ ┻━┻

mosra commented 5 years ago

@marzer yay, thanks for the confirmation!

What does it break for you, in particular? Are these in any way related to https://github.com/doxygen/doxygen/issues/6714 or https://github.com/doxygen/doxygen/issues/6715 that I discovered today after upgrading to 1.8.15 myself?

marzer commented 5 years ago

@mosra ah, I'm not sure, to be honest- I'm not using either of those features. The main issue I'm experiencing with 1.8.15 is a weird regression in how doxy is parsing namespaces; some of the types in our codebase (a very small number, around 8 or so) are seeing the top-level namespace duplicated an apparently arbitrary number of times, e.g.

Achilles::Vertices::ScopedFormat

is interpreted as

Achilles::Achilles::Achilles::Achilles::Achilles::Achilles::Achilles::Achilles::Achilles::Achilles::Vertices::ScopedFormat

which then results in

struct_achilles_1_1_achilles_1_1_achilles_1_1_achilles_1_1_achilles_1_1_achilles_1_1_achilles_1_29f9696f4e9bfa24b1b4e2d56e8ccc91.html

=/

mosra commented 5 years ago

Wow! What the shit. I'm afraid I didn't see this one yet (but I saw a lot of weird shit happening in Doxygen, so this is totally possible).

Achilles:: is 10 characters and it's repeated 10 times. Coincidence? I bet not! :laughing:

I would suggest reporting a but to doxygen, but from my experience a bugreport has to contain a minimal repro case (and/or a bisected commit) in order to not get completely ignored, and even then it can take years to fix unless you complain a lot. Since your codebase is private:

Are you able to build doxygen locally and git bisect to a particular commit that caused this to happen between 1.8.14 and 1.8.15? Seeing the commit might give a hint to what could be the culprit.
Since it's happening in a relatively few cases, try to minimize one of these? The bisected commit might point to a particular problematic area.

I wish you good luck and enough patience with reporting this. I spent 14 hours yesterday on similar pointless regressions caused by careless coding practices by doxygen maintainers and I'll never get that time back. At this point, I'm seriously considering writing a Doxygen replacement.

marzer commented 5 years ago

@mosra that is a great idea, and one I'd eagerly support. At some point I was vaguely entertaining the idea of forking doxygen, stripping out everything not related to XML, and working forward from there, but upon reflection I realized it's probably easier to start from scratch and build an XML-generating clang front-end for it instead.

mosra commented 5 years ago

There's a fork, Doxypress, but I would first need to re-apply all my XML-related fixes and improvements to make it even remotely usable for this theme.

mosra / m.css

Doxygen: Extra <p><br /> added when a node contains multiple \remarks, \warning, etc. #47