-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
Fix bug reported in Issue #441 #445
Conversation
From the bug report it is a bit unclear to me how much of the function signature Breathe gives to Sphinx, but it looks like the whole arrow+type is missing. |
@jakobandersen: Thanks for the quick answer. I have no problem working on a solution so that Sphinx gets the right method declaration. But unfortunately it is not yet clear to me how Breathe and Sphinx work together. Could you help me to understand what exactly Sphinx would expect from Breathe (declaration / definition) and where I can find the necessary structures in the Breathe code? At the moment it's only somewhat clear to me how Breathe is parsing Doxygen's XML. |
@jakobandersen: I found out where the signature for Sphinx is generated and made the adjustment for trailing return types accordingly (see force-pushed commit). Is this as you expected it to be? |
Slight push for everyone who could possibly help me with my problem :) |
@Sonicsystem From my point of view, as the current Breathe maintainer, I do agree with Jakob's post to fix this properly upstream in Sphinx. I believe it should be fixed in the C++ domain, Breathe can then pass the full signature to Sphinx. Not sure if this has been done yet, but it may be a good idea to open up an issue on Sphinx's issue tracker as well. You could also consider writing the patch for the C++ domain, depending on how busy everyone is. Personally I am not activate in Sphinx, so I cannot say much more. If the issue is resolved upstream, this MR can then proceed to be merged, likely after a revision. |
Was there ever an issue created at sphinx's side? I believe this is fine to merge for breathe, but I would like to play it safe and time the merges properly to avoid any breakage due to mismatching versions. Thoughts? |
Unfortunately, i have no idea if there was ever an issue created at sphinx's side relating to this problem. I did not open one as we have been fine with a slight custom made fix just in the breathe code. As long as the problem is not fixed at sphinx side, it does not really make sense to merge this fix probably. But it also does not seem that too many people have a problem with trailing return types and sphinx + breathe :) |
FYI I am considering closing this one soon as it has reached a stand still and I am not sure what to do with it. |
I think #512 incidentally subsumed this PR, though it did indeed solve the original issue before. |
In that case I think it is safe to assume #512 solved the issue. @nicokoe If you could test and let us know that would be great, see also #441 (comment). |
A small fix to the bug which is reported in issue #441 with c++'s trailing return types.