search

brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.72k stars 2.31k forks source link

Support non-const references as parameters in cpplint #22265

Open onyb opened 2 years ago

onyb commented 2 years ago

Google recommends the following style as a best practice.

Prefer to return by value or, failing that, return by reference. Avoid returning a pointer unless it can be null. Non-optional input parameters should usually be values or const references, while non-optional output and input/output parameters should usually be references (which cannot be null).

This is not consistent with the current cpplint rules, which shows the following error when using non-const references in function arguments:

Is this a non-const reference? If so, make const or use a pointer: std::string& arg  [runtime/references]

cc: @supermassive

bridiver commented 2 years ago

@onyb if you're referring to https://github.com/brave/brave-core/pull/12797/files#diff-832b548af9f742c2cd91f0f70f8e85102e93c3ca4ded730672f894b1d139d580R273 then you're misreading the docs. That is referring to return values, not params. Also we use upstream cpplint rules so if they're wrong, it's wrong in upstream.

bridiver commented 2 years ago

closing as won't fix because we are consistent with upstream

supermassive commented 2 years ago

@onyb if you're referring to https://github.com/brave/brave-core/pull/12797/files#diff-832b548af9f742c2cd91f0f70f8e85102e93c3ca4ded730672f894b1d139d580R273 then you're misreading the docs. That is referring to return values, not params. Also we use upstream cpplint rules so if they're wrong, it's wrong in upstream.

That was a reference to foo(std::string* out) vs foo(std::string& out) discussion which we had in that PR.

Upstream linter actually has that rule disabled via filters set for cpplint object: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2248714/4/presubmit_canned_checks.py#44 and we don't follow that behavior.

I believe this is worth fixing, so reopening @bridiver

bridiver commented 2 years ago

@supermassive ok, I see where we are missing that now because they are passing those to cpplint instead of changing the cpplint.cfg. We can do something similar in lint.py or maybe we can look at running presubmit checks instead