-
Notifications
You must be signed in to change notification settings - Fork 87
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
Doxygen: Extra <p><br /> added when a node contains multiple \remarks, \warning, etc. #47
Comments
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 |
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? |
Hum, sorry, can't reproduce. This is how my test looks like (whitespace highlighted): This is the generated XML output for the <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? |
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 |
RE things you might have missed: O_o |
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. |
I just copied the above snippet verbatim, removed 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:
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... :/ |
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. |
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 :) |
@marzer if you got some time to spare, I just pushed a branch named 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. |
Sure thing:
|
Argh, platform-dependent Doxygen defaults 💥 💀 🔫 ... Can you try again with 24ff250? Thank you and sorry for mess. |
|
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 :) |
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. |
No worries, thanks for trying ^_^ |
@mosra good news: doxygen 1.8.15 fixes this issue |
@marzer yay, thanks for the confirmation! What does it break for you, in particular? Are these in any way related to doxygen/doxygen#6714 or doxygen/doxygen#6715 that I discovered today after upgrading to 1.8.15 myself? |
@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.
is interpreted as
which then results in
=/ |
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).
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:
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. |
@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. |
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. |
See title. Essentially, this:
Now has an extra
<p><br />
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!)
The text was updated successfully, but these errors were encountered: