In D79113#2016294, @aaron.ballman wrote:

I only see a comment on the issue, but not that the issue was resolved in favor of that comment (unless I've missed something). If the issue gets resolved in the direction that comment is going, then the revert makes sense (though we should add an explicit test case showing that we want the diagnostic).

Removed redundant condition in check

Moved previously added condition into the actual matcher.

Ran automatic formatting on the patch

Modified the check to not report static variables in function scope.

In D77461#1977703, @aaron.ballman wrote:

In D77461#1977687, @vingeldal wrote:

In D77461#1977639, @aaron.ballman wrote:

In D77461#1977503, @vingeldal wrote:

In D77461#1963166, @lebedev.ri wrote:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

I don't follow your train of thought. Could you please elaborate on why you think this must be a false positive?

I think it must be a false positive because the rule is about global variables where "global" refers to the scope of the variable. This is a locally scoped variable. Also, the rule's enforcement section is pretty clear that this does not apply to local statics.

My reason for hesitating to call this a false positive is that this pattern does cause a hidden dependency between users of the function, hence it clearly goes against the short and simple rationale given for this rule:
"Non-const global variables hide dependencies and make the dependencies subject to unpredictable changes."

I don't see it hiding a dependency between users of the function. The local static could be swapped out for something else (an actual global, linker magic, etc) and the caller would be unaware. The same is not true for a globally scoped object where the identifier is exposed because someone else could be trying to link to that symbol (and they would break if the symbol changed).

A caller of this function will clearly be affected by other callers if there are any, so there clearly is a dependency added between callers of this function. At the same time the function doesn't provide any hints toward the fact that it is statefull or any way for the caller achieve observability of this state, hence the state causing the dependency is hidden and the dependency is hidden.

I must agree that the enforcement section goes against my interpretation but I would prefer to try to simply change the enforcement section.

This function with a static variable can almost always be replaced with a solution where the free function is instead made a member function of a class keeping the state as a non-static, private member variable -which would more clearly convey that the function may have state and would provide stronger encapsulation.
The only exception would be if we are talking about a C-interface, in which case one could still use a class, as I just described, instead of a static variable; you would just have to call the member function via a free function.
Can anyone explain to me why one would ever *have* to rely on functions with static variables?

To avoid the static initialization order fiasco, statistics or timing counters, etc -- look through the LLVM code base for function local statics, they're not uncommon.

Because before I can see a legitimate use case for this potential false positive I think it would be a bad idea to consider it a false positive since there are obvious issues with hidden dependencies in the discussed example code.

If we don't hear back from the C++ Core Guidelines authors quickly on their thoughts or adjustments, there are at least two reviewers who consider it a false positive based on the current rule text, which is sufficient post-review feedback to warrant a change IMO.

I'm also adding to this discussion the exception-part of the rule:
"A global object is often better than a singleton."

In D77461#1977639, @aaron.ballman wrote:

In D77461#1977503, @vingeldal wrote:

In D77461#1963166, @lebedev.ri wrote:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

I don't follow your train of thought. Could you please elaborate on why you think this must be a false positive?

I think it must be a false positive because the rule is about global variables where "global" refers to the scope of the variable. This is a locally scoped variable. Also, the rule's enforcement section is pretty clear that this does not apply to local statics.

My reason for hesitating to call this a false positive is that this pattern does cause a hidden dependency between users of the function, hence it clearly goes against the short and simple rationale given for this rule:
"Non-const global variables hide dependencies and make the dependencies subject to unpredictable changes."

I don't see it hiding a dependency between users of the function. The local static could be swapped out for something else (an actual global, linker magic, etc) and the caller would be unaware. The same is not true for a globally scoped object where the identifier is exposed because someone else could be trying to link to that symbol (and they would break if the symbol changed).

In D77461#1963166, @lebedev.ri wrote:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

After looking in to it I got less certain of where the error lies and eventually I got uncertain if this even is a false negative, so I started out with just posting a change where the unit test is updated to detect the issue.
This is just to have a start for a discussion of how this should actually work and what is the best way forward.

After looking more closely at the code I think the issue is within hasLocalStorage() which is called in hasGlobalStorage(). My expectation would be that anything inside of function scope would be considered local but I'm not very certain.
Any thoughts on whether hasLocalStorage() should be modified or if I should change the check and use some more ad-hoc implementation, instead of hasGlobalStorage(), to determine if the variable is local or global?

In D70265#1953335, @lebedev.ri wrote:

Hello.
2 points:

rG512767eb3fe9c34c655a480d034147c54f1d4f85 doesn't reference this review,
so it's a bit hard to find. Please try to reference it next time.

Will do

In D69560#1935071, @whisperity wrote:

@aaron.ballman I've gone over LLVM (and a few other projects). Some general observations:

• Length of 2 is vile. I understand that the C++CG rule says even lengths of 2 should be matched, but that is industrially infeasible unless one introduces such a rule incrementally to their project. Findings of length 2 are , in general, an order of magnitude more than... basically the rest of the findings.
  • On big projects, even the current "default" of "length 3" seems to be too low. In reality, one should consider (this is likely to be out of scope for Tidy) how often these functions are called, and various other metrics on how serious an offender is.

In D70265#1920379, @aaron.ballman wrote:

LGTM! Do you need someone to commit on your behalf?

Made example code in documentation legal C++

In D69560#1890026, @aaron.ballman wrote:

In D69560#1889925, @vingeldal wrote:

Doesn't clang-tidy exclude warnings from system headers by default though?

Third-party SDKs are not always brought in as system headers, unfortunately. Even ignoring system and third-party headers, having two parameters of the same type is a natural occurrence for some data types (like bool) where it is going to be extremely hard to tell whether they're related or unrelated parameters.

And there is always the possibility of using multiple .clang-tidy files in the code base to use this check for only selected portions of the code base.

That is an option, but it would be better for the check to be usable without requiring a lot of custom configuration.

One can't really expect a ones own .clang-tidy file to be applicable to all third party code used in ones project. It is a fact, with or without this check, that if you use third party libraries you can't expect the third party libraries to comply with your own .clang-tidy file. You should expect to set up a separate .clang-tidy file for all third party libraries used by your project with or without this check -and that's ok because it is arguably not a big effort to do so.

In D69560#1889719, @aaron.ballman wrote:

In D69560#1889483, @whisperity wrote:

I'd rather not call them false positive because the definition of false positive is ugly and mushy with regards to this check. This check attempts to enforce an interface rule, whether you(r project) wants to adhere the rule is a concise decision. A type equivalence (or convertibility in case of the follow-up patch D75041 that considers implicit conversions) should be a "true positive" in every case, as an indicator of potentially bad design.

The problem is that there *are* false positives, logically. The rule is about "unrelated" parameters, so the false positives come from not having any way to distinguish between related and unrelated arguments. My intuition is that the simple enforcement suggested by the rule is unlikely to be acceptable, but that's why I'm asking for data over large, real-world projects.

I agree with this, the way I see it there are false positives and there is nothing "mushy" about the definitions of those.
Though I would prefer that, even if the implementation suggested in the guideline would be to crude, any deviations from what the C++ Core Guidelines specifies must be explicitly chosen by the user and not an option default.

Nice! Do you have any numbers on the amount of false positives this check produces, or the ratio of false and true positives?
I recently started work on this check myself but didn't get very far before I discovered this patch and abandoned mine.
Before abandoning I got some feedback though; there were concerns that there would be way to many false positives.

Will proceed with instead contributing however I can to the more mature patch https://reviews.llvm.org/D69560

Discovered duplication: https://reviews.llvm.org/D69560

In D74463#1889157, @vingeldal wrote:

• how do we either not warn on this by default or how does the user tell us to not warn on it (without requiring them to jump through hoops like changing the types of the arguments)?

-I'v used comments in the source code to tell the tool to ignore cases that I'v identified as false positives. That has worked without any issues for me and I wouldn't say it's a hassle. Is that no longer supported in clang tidy or was I using another tool and just projected that memory on clang-tidy?
I'm confident that clang-format atleast has a means of locally suppressing rules.

• how do we either not warn on this by default or how does the user tell us to not warn on it (without requiring them to jump through hoops like changing the types of the arguments)?

-I'v used comments in the source code to tell the tool to ignore cases that I'v identified as false positives. That has worked without any issues for me and I wouldn't say it's a hassle. Is that no longer supported in clang tidy or was I using another tool and just projected that memory on clang-tidy?
I'm confident that clang-format atleast has a means of locally suppressing rules.

In D74463#1878378, @njames93 wrote:

In D74463#1878290, @vingeldal wrote:

I am pretty sure that an option to allow short names would cause a relatively big hit on performance (relative to how it runs without the option) for this check while also potentially causing some false negatives (which I would very much like to avoid).

Highly unlikely, the additional name length check would only be ran on functions where the consecutive param types occur, then the check itself is very fast, you don't even need to compute the length as its stored in the identifier info, finally on cases where the name check prevents a diagnostic that is a huge performance win. The false negatives could kind of be an issue, but we could leave that up to the developer who is running the check.

I still feel a bit reluctant since the potential false positives would at least be left to the user to decide how to handle, while the false negatives from this option would just be lost without indication.
I don't see much value left in this check if one decides to use the option, anyone who would use this option might be better of just doing this check manually instead, then again it's optional of course.
If there wouldn't be much of a loss in performance I guess there isn't any significant harm in adding the option though, I'll give it a shot.

In D74463#1878144, @njames93 wrote:

I have a feeling this check is going to throw so many false positives that it'll be too noisy to run on any real codebase.
There should be a way to silence this when it is undesired like in the example for int max(int a, int b);.
A possible solution could be based on parameter name length, usually interchangeable parameters have short names.
Maybe an option could be added IgnoreShortParamNamesSize which takes an int and disregards results if both params are shorter, set as 0 to disable the option

Updating D74463: Add new check avoid-adjacent-unrelated-parameters-of-the-same-type

The pull request to C++ Core guidelines have been merged: https://github.com/isocpp/CppCoreGuidelines/pull/1553

Updating D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check

In D71963#1799956, @aaron.ballman wrote:

I may have missed this in prior discussions, and if so, I'm sorry -- but why are we taking CodeChecker as the model for this? I'm not opposed to having some notion of severity for our checks (basically every tool and many coding standards have the same concept), but I am not certain why we would say CodeChecker's way is the way we want clang-tidy to progress. Also, does this tie in with the clang static analyzer checks, or is the severity stuff only for clang-tidy?

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

@whisperity I think you misunderstood my comment. I was not trying to give a more correct description of the current definition of style-level severity in CodeChecker. I was trying to propose new definitions of the different severity levels that this patch propose for clang-tidy.
So the claim that my suggestion is not "true" is invalid since that isn't a matter of true or false, but merely a matter of how one defines the style level.

It's very hard to write these kinds of definitions without ambiguity and plenty of subjective interpretation creeping in. I'll try my best to provide constructive feedback but I'm admittedly struggling a bit with providing helpful counter proposals.
Ideally these levels would be based on a data driven approach, like when we say that something is error prone we should be able to support that with data.

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines