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

doxygen: fix keywords leaking into return type #228

Closed
wants to merge 1 commit into from

Conversation

marzer
Copy link
Contributor

@marzer marzer commented Apr 30, 2022

Hola! I have done some work on doxygen.py/parse_func() to fix #225 and #226 by unifying the various ways the function handles the cases were doxygen leaks some keywords into the return type. It now should work regardless of the order they leak in and/or if they appear at the beginning or end of the type, as well as being more robust in the face of future Doxygen fuckups regressions. Brute force, yo.

Also did some drive-by fixes:

  • Added support for C++20's consteval (since mimicking the way constexpr was handled for functions seemed easy enough)
  • Fixed a typo in CONTRIBUTING.rst

I've updated the doxygen tests, too. When I run them there are a number of test failures completely unrelated to functions (inline namespaces and such), but they appar to be known issues with Doxygen 1.9. They pass when I select only the ones that are relevant to my changes:

python -m unittest discover -p test_cpp.py
python -m unittest discover -p test_compound.py

@marzer
Copy link
Contributor Author

marzer commented Apr 30, 2022

Oh, interesting, looks like there might be some other test HTML I've forgotten to update I'm a dumbass. Hold please.

@marzer marzer force-pushed the doygen-func-keyword-fixes branch from 5248267 to 5b0691b Compare April 30, 2022 09:50
@codecov
Copy link

codecov bot commented Apr 30, 2022

Codecov Report

Merging #228 (89bffa9) into master (6f5c7d5) will increase coverage by 0.00%.
The diff coverage is 93.10%.

@@           Coverage Diff           @@
##           master     #228   +/-   ##
=======================================
  Coverage   98.15%   98.16%           
=======================================
  Files          27       27           
  Lines        6951     6963   +12     
  Branches       49       49           
=======================================
+ Hits         6823     6835   +12     
  Misses        128      128           
Impacted Files Coverage Δ
documentation/doxygen.py 99.04% <93.10%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f5c7d5...89bffa9. Read the comment docs.

@marzer
Copy link
Contributor Author

marzer commented Apr 30, 2022

There we are. Should be good now.

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Thank you! Finally managed to squeeze out some free time from my schedule to look at this.

I hope it's not too many comments, heh.

CONTRIBUTING.rst Outdated Show resolved Hide resolved
documentation/templates/doxygen/details-func.html Outdated Show resolved Hide resolved
documentation/templates/doxygen/entry-func.html Outdated Show resolved Hide resolved
documentation/doxygen.py Outdated Show resolved Hide resolved
documentation/doxygen.py Outdated Show resolved Hide resolved
documentation/doxygen.py Show resolved Hide resolved
documentation/doxygen.py Outdated Show resolved Hide resolved
@mosra mosra linked an issue May 12, 2022 that may be closed by this pull request
@marzer marzer force-pushed the doygen-func-keyword-fixes branch 4 times, most recently from 9997ffa to 33a8f63 Compare May 12, 2022 18:24
@marzer marzer requested a review from mosra May 12, 2022 18:25
@marzer marzer force-pushed the doygen-func-keyword-fixes branch from 33a8f63 to b1c2b56 Compare May 13, 2022 14:51
- fixes mosra#225
- fixes mosra#226

Also drive-by added support for C++20's `consteval` keyword.
@marzer marzer force-pushed the doygen-func-keyword-fixes branch from b1c2b56 to 89bffa9 Compare May 13, 2022 14:56
@mosra
Copy link
Owner

mosra commented May 13, 2022

Rebased and merged as 2add9e0, thanks a lot for your patience :)

@mosra mosra closed this May 13, 2022
@marzer
Copy link
Contributor Author

marzer commented May 13, 2022

Rebased and merged as 2add9e0, thanks a lot for your patience :)

No worries, happy to help improve the lib 😄

As an aside/out of curiousity: what's the deal with PR merges on this repo? They're closed but then still somehow merged in anyways. Some git/tooling shenanigans?

@mosra
Copy link
Owner

mosra commented May 13, 2022

I pull the branch locally and rebase it on top of master first, to avoid a merge commit.

Historically GitHub didn't have UI to do that for me without squashing all commits together (maybe it does now? never actually bothered checking, GitLab used to have that as a paid feature), plus usually I also do a final pass over the changes locally before finally pushing them to master. To make the PR appear as merged, I'd need to push the rebased commits back to your repo, and for obvious reasons ("what the hell happened to my fork while I was not looking??") I'm not doing that :D

@crisluengo
Copy link
Contributor

There is a "Rebase and merge" option.

@marzer marzer deleted the doygen-func-keyword-fixes branch May 14, 2022 10:49
marzer added a commit to marzer/m.css that referenced this pull request Oct 5, 2022
also:
- fixed `static` and `constexpr` leaking into variables (basically the same problem as mosra#228)
- fixed a few tests that were broken with Doxygen 1.9.0+
- added support for C++20's `constinit`
marzer added a commit to marzer/m.css that referenced this pull request Oct 16, 2022
also:
- fixed `static` and `constexpr` leaking into variables (basically the same problem as mosra#228)
- fixed a few tests that were broken with Doxygen 1.9.0+
- added support for C++20's `constinit`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
3 participants