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

Address #7491 - Configuration of pointer/ref placement in C++ decls #7507

Closed
wants to merge 2 commits into from

Conversation

vector-of-bool
Copy link

Subject: Allow configuration of the placement of the reference and pointer tokens in C++ declarations in the rendered output.

Feature or Bugfix

  • Feature - 2 additional configuration options, no changes for existing projects

Purpose

  • This change adds options cpp_ptr_placement and cpp_ref_placement, which determine where the rendered output will attach the reference & or pointer * declaration token relative to the main type and the declared name. "right" (the current behavior and default) will attach towards the right on the declared name, whereas "left" will attach towrads the left, closer to the pointed-to/referred type.

Relates

This change adds options 'cpp_ptr_placement' and 'cpp_ref_placement',
which determine where the rendered output will attach the reference
`&` or pointer `*` declaration token relative to the main type and
the declared name. "right" (the current behavior and default) will
attach towards the right on the declared name, whereas "left" will
attach towrads the left, closer to the pointed-to/referred type.
@jbab
Copy link
Contributor

jbab commented Apr 26, 2020

Thanks so much, I really appreciate this.

From a quick glance and a smoke test on my personal, very rudimentary C++-docs-with-sphinx attempts, results look good, except for one thing: I would expect the ampersand in a function ref-qualifier to be attached to the const (or volatile) in the left mode: int f() const&, not int f() const &.

As for the names of the options, I like them, they are neutral and convey their meaning. Also specifying the option value as an enum is a plus; in this way future extension are possible, e.g. a value "middle".

Having said that I am still a little concerned about the many output variations this results in and the impact this has on testing and maintenance. The crucial question is what is really needed. There must be a reason that another widely-used tool, clang-format, fares well with only one option (and three values): PointerAlignment.

@vector-of-bool
Copy link
Author

I thought about reducing it to a single option for both, and maybe that's a good idea. I don't know of any codebases that mix-and-match the alignment of their * and & tokens. There's no compelling reason they can't be consolidated to a single option.

I didn't consider the case with ref-qualified functions. I'll look into that one.

Comment on lines +554 to +557
if val not in ('left', 'right'):
raise RuntimeError('Invalid C++ placement parmeter value '
'"{}", should be either "left" or "right"'.format(val))
return _Placement(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

Marginally faster as

Suggested change
if val not in ('left', 'right'):
raise RuntimeError('Invalid C++ placement parmeter value '
'"{}", should be either "left" or "right"'.format(val))
return _Placement(val)
try:
return _Placement(val)
except ValueError:
raise RuntimeError('Invalid C++ placement parmeter value '
'"{}", should be either "left" or "right"'.format(val)) from None

abrauninger added a commit to abrauninger/sphinx that referenced this pull request Dec 24, 2020
https://github.com/vector-of-bool/sphinx commit cfe8645, modulo a few changes; `middle` doesn't work yet
@tk0miya tk0miya closed this May 9, 2021
@tk0miya tk0miya deleted the branch sphinx-doc:3.x May 9, 2021 03:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants