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

Non-const reference parameters are still not allowed #148

Open
achanana opened this issue Jun 5, 2020 · 5 comments
Open

Non-const reference parameters are still not allowed #148

achanana opened this issue Jun 5, 2020 · 5 comments

Comments

@achanana
Copy link

achanana commented Jun 5, 2020

The Google C++ Style Guide seems to have allowed using non-const references as parameters, I see not reason why not to allow them here too. (runtime/references)

Relevant styleguide changes (google/styleguide@57cd341...gh-pages):

- Input parameters are usually values or <code>const</code> references, while output and input/output parameters will be non-<code>const</code> pointers.
+ Input parameters should usually be values or <code>const</code> references, while required (non-nullable) output and input/output parameters should usually be references.

- All parameters passed by reference must be labeled <code>const</code>.
- In C, if a function needs to modify a variable, the parameter must use a pointer, eg <code>int foo(int *pval)</code>. In C++, the function can alternatively declare a reference parameter: <code>int foo(int &amp;val)</code>
- Within function parameter lists all references must be
<code>const</code>:
- In fact it is a very strong convention in Google code that input arguments are values or <code>const</code> references while output arguments are pointers. Input parameters may be <code>const</code> pointers, but we never allow non-<code>const</code> reference parameters except when required by convention, e.g., <code>swap()</code>.

Most changes seem to come from this recent commit:
google@7a7a2f510efe7d7fc (May 20th, 2020)

@tkruse
Copy link

tkruse commented Jun 6, 2020

This would also be a good ticket to open at upstream https://github.com/google/styleguide/issues. Possibly the google internal cpplint version already has such a change.

Generally speaking, there are multiple sources of truth here:

While I would like to make cpplint adhere to the styleguide (or the internal google cpplint version), the convention for this project is to follow the public cpplint.py for default behavior, and only add bugfixes or fixes to make cpplint useful in other contexts than google code. This is also important to be able to smoothly merge upstream changes to public google cpplint (which are not very frequent).

Since users can already disable runtime/references manually, not sure if more needs to be done at this time.

@wp-seth
Copy link

wp-seth commented Jul 29, 2020

a corresponding issue at google is google#563.
i'll call the different sources (and this target):

  • public styleguide
  • internal google cpplint (devtools/cpplint/cpplint.py)
  • public google cpplint (github.com/google/styelguide/...)
  • public modern cpplint (this repo)

the public google cpplint.py is very old and outdated. since 2019-11-19 there were no updates (apart from global replacements of recently blacklisted words such as "blacklist"). it might have been a good starting point a long time ago. but as it is not synchroneous to the public styleguide, it is (at least temporary) useless now.
the internal google cpplint is not public, so in fact it is not existent for the public and thus can't be a reasonable reference for a open source project.
so i agree with @tkruse: the public styleguide should be seen as the base of the public modern cpplint as it is public and more modern than the public google cpplint.
and so i'd appreciate, if the bug mentioned by @achanana would be fixed. in particular i'd call the old rule (which is still implemented in both public cpplints) bad cpp style.

@caioaamaral
Copy link

maybe you could start a 2.0 version with modern cpplint and keep 1.x.x as legacy cpplint (that way all projects that depends on this cpplint would have an option to upgrade or not).

@carlocorradini
Copy link

Any update? 😰

@eguiraud
Copy link

you could start a 2.0 version with modern cpplint and keep

As things currently are, it seems to me that any C++ project that follows the Google C++ style guide will have to turn off runtime/references checks, so I would not necessarily wait for 2.0 to fix this 😬

I agree with @wp-seth that "the public styleguide should be seen as the base of the public modern cpplint", since that's what the vast majority of projects using cpplint will be following.

HaoZeke added a commit to HaoZeke/readCon that referenced this issue Apr 25, 2024
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

No branches or pull requests

6 participants