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

Make doxygenfunction more robust when matching parameters #723

Merged

Conversation

jakobandersen
Copy link
Collaborator

@jakobandersen jakobandersen commented Aug 5, 2021

(This depends on / includes #698)

Fixes the basics of #722, and all(?) other cases where information does not contribute to overload resolution. See examples/specific/cpp_function_lookup.h for examples (https://github.com/michaeljones/breathe/pull/723/files?file-filters%5B%5D=.h&file-filters%5B%5D=.py#diff-b4e4a1134077e09d357e73a9c4ca3cb49fec09a3264c88bc5119aeaa48990721).

Things that may still fail:

  • Two functions only differ in their return type (e.g., a trailing return type with decltype(expr) which would make all but one overload valid. Currently the trailing return type is stripped. The syntax of doxygenfunction needs to be expanded to properly handle this.
  • Two functions only differ in their template parameter list or (trailing) requires clause. The syntax of doxygenfunction needs to be expanded to properly handle this.
  • An "array" parameter which is equivalent to a pointer parameter (int *p vs. int p[]). The user must use the same syntax as in the code. I'm not sure if it's worth making this work.

@jakobandersen jakobandersen changed the title [WIP'ish] Fix doxygenfunction for named function pointers Make doxygenfunction more robust when matching parameters Aug 7, 2021
@jakobandersen
Copy link
Collaborator Author

Now updated with tests and more systematic stripping of information that doesn't contribute to overloading.

@aprotyas
Copy link

@jakobandersen just curious: are the changes in this PR independent of those in #711? I believe I need both sets of patches for my use case, so I was wondering if I could just rebase one on the other and install locally.

@jakobandersen
Copy link
Collaborator Author

@jakobandersen just curious: are the changes in this PR independent of those in #711? I believe I need both sets of patches for my use case, so I was wondering if I could just rebase one on the other and install locally.

Yes, they should be. As they both include the commits of #698 it may be icky to do it as a rebase, but I just tried locally to merge the two branches, which should work without problems.

aprotyas pushed a commit to aprotyas/breathe that referenced this pull request Aug 19, 2021
Note, both of these PRs are on top of the changes introduced by
jakobandersen#698.

Links to PRs for future reference:
- breathe-doc#698
- breathe-doc#711
- breathe-doc#723

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
aprotyas pushed a commit to aprotyas/breathe that referenced this pull request Aug 19, 2021
Note, both of these PRs are on top of the changes introduced by
upstream PR breathe-doc#698.

Links to PRs for future reference:
- breathe-doc#698
- breathe-doc#711
- breathe-doc#723

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
@michaeljones
Copy link
Collaborator

The only issue I can see is the space between the * and the arg:

image

But maybe that is fine. I'm out of touch with C++ too at the moment 😅

@aprotyas
Copy link

aprotyas commented Aug 21, 2021

The only issue I can see is the space between the * and the arg:

image

But maybe that is fine. I'm out of touch with C++ too at the moment 😅

I believe this is fine, though I don't have a proper reference to provide.

@michaeljones
Copy link
Collaborator

@jakobandersen - are you happy for this to be merged? Also should I make you a collaborator on the repo? I assume that has come up before but I'm surprised that you're not.

@jakobandersen
Copy link
Collaborator Author

The only issue I can see is the space between the * and the arg:

image

But maybe that is fine. I'm out of touch with C++ too at the moment

Indeed, this is an icky case, and in the end a matter of style. Though, this is the rendering currently defined in Sphinx, where I generally put the declarators with the name, e.g., int *p instead of int* p. But for member pointers there is position for a space on the left, and int A::*p seemed a bit too compact for my taste. The issue sphinx-doc/sphinx#7491 is rather related, but there it is non-trivial to make the spacing user-configurable without a lot of haxing.

@jakobandersen
Copy link
Collaborator Author

@jakobandersen - are you happy for this to be merged? Also should I make you a collaborator on the repo? I assume that has come up before but I'm surprised that you're not.

In principle it is ready to merge, but as it includes #698 we should get that one fixed first. Though, I'd like to rebase it so the CI can run through.

You are welcome to make me collaborator. I'll probably only merge non-trivial things my self and keep the previous workflow with at least one reviewer for the larger things.

@jakobandersen jakobandersen force-pushed the func_lookup_function_pointer branch 3 times, most recently from 4c0b7a8 to b3ec822 Compare September 2, 2021 06:40
@jakobandersen
Copy link
Collaborator Author

Note to self, need update after the parsing fix has been merged.

@jakobandersen jakobandersen merged commit 3023db0 into breathe-doc:master Sep 2, 2021
@jakobandersen jakobandersen deleted the func_lookup_function_pointer branch September 2, 2021 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants