Page MenuHomePhabricator

vingeldal (Kim Viggedal)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 13 2019, 10:14 PM (54 w, 4 d)

Recent Activity

May 4 2020

vingeldal added a comment to D79113: Revert "Remove false positive in AvoidNonConstGlobalVariables.".

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).

May 4 2020, 12:21 PM · Restricted Project

Apr 29 2020

vingeldal added reviewers for D79113: Revert "Remove false positive in AvoidNonConstGlobalVariables.": aaron.ballman, lebedev.ri, JonasToth, gribozavr2.
Apr 29 2020, 12:22 PM · Restricted Project
vingeldal created D79113: Revert "Remove false positive in AvoidNonConstGlobalVariables.".
Apr 29 2020, 12:22 PM · Restricted Project
vingeldal added a reverting change for rGb2d8c89ea48b: Remove false positive in AvoidNonConstGlobalVariables.: D79113: Revert "Remove false positive in AvoidNonConstGlobalVariables.".
Apr 29 2020, 12:22 PM

Apr 14 2020

vingeldal updated the diff for D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

Removed redundant condition in check

Apr 14 2020, 11:50 AM · Restricted Project
vingeldal added inline comments to D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.
Apr 14 2020, 11:50 AM · Restricted Project
vingeldal updated the diff for D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

Moved previously added condition into the actual matcher.

Apr 14 2020, 8:00 AM · Restricted Project
vingeldal added inline comments to D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.
Apr 14 2020, 8:00 AM · Restricted Project

Apr 13 2020

vingeldal updated the diff for D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

Ran automatic formatting on the patch

Apr 13 2020, 12:25 PM · Restricted Project
vingeldal updated the diff for D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

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

Apr 13 2020, 12:25 PM · Restricted Project
vingeldal added a comment to D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

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.

Apr 13 2020, 6:57 AM · Restricted Project
vingeldal added a comment to D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

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

Apr 13 2020, 6:25 AM · Restricted Project
vingeldal added a comment to D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

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).

Apr 13 2020, 6:25 AM · Restricted Project
vingeldal added a comment to D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

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.

Apr 13 2020, 2:39 AM · Restricted Project

Apr 4 2020

vingeldal added a comment to D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.

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.

Apr 4 2020, 7:56 AM · Restricted Project
vingeldal created D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.
Apr 4 2020, 7:56 AM · Restricted Project

Apr 1 2020

vingeldal added a comment to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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?

Apr 1 2020, 8:14 AM · Restricted Project

Mar 31 2020

vingeldal added a comment to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Mar 31 2020, 10:32 PM · Restricted Project

Mar 23 2020

vingeldal added a comment to D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check.

@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.
Mar 23 2020, 12:31 AM · Restricted Project, Unknown Object (Project)

Mar 13 2020

vingeldal added a comment to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Mar 13 2020, 12:08 AM · Restricted Project

Mar 12 2020

vingeldal updated the diff for D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

Made example code in documentation legal C++

Mar 12 2020, 10:51 AM · Restricted Project
vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Mar 12 2020, 10:51 AM · Restricted Project

Mar 10 2020

vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Mar 10 2020, 11:25 AM · Restricted Project

Feb 24 2020

vingeldal added a comment to D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check.

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.

Feb 24 2020, 11:56 PM · Restricted Project, Unknown Object (Project)
vingeldal added a comment to D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check.

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.

Feb 24 2020, 12:11 PM · Restricted Project, Unknown Object (Project)
vingeldal added a comment to D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check.

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.

Feb 24 2020, 7:17 AM · Restricted Project, Unknown Object (Project)
vingeldal abandoned D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.

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

Feb 24 2020, 6:50 AM · Restricted Project
vingeldal added a comment to D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.

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

Feb 24 2020, 6:41 AM · Restricted Project
vingeldal added a comment to D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.
  • 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.

Feb 24 2020, 6:31 AM · Restricted Project
vingeldal added a comment to D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.
  • 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.

Feb 24 2020, 6:22 AM · Restricted Project

Feb 23 2020

vingeldal added a comment to D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.

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.

Feb 23 2020, 5:59 AM · Restricted Project

Feb 16 2020

vingeldal added a comment to D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.

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

Feb 16 2020, 9:24 AM · Restricted Project

Feb 11 2020

vingeldal added reviewers for D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type: aaron.ballman, JonasToth.
Feb 11 2020, 11:30 PM · Restricted Project
vingeldal updated the diff for D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.

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

Feb 11 2020, 11:23 PM · Restricted Project
vingeldal created D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.
Feb 11 2020, 11:12 PM · Restricted Project
vingeldal added a comment to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Feb 11 2020, 1:23 AM · Restricted Project
vingeldal updated the diff for D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Feb 11 2020, 1:11 AM · Restricted Project

Jan 6 2020

vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Jan 6 2020, 1:58 AM · Restricted Project

Dec 31 2019

vingeldal added a comment to D71963: clang-tidy doc: Add the severities description.

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?

Dec 31 2019, 7:14 AM · Restricted Project

Dec 30 2019

vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Dec 30 2019, 7:07 AM · Restricted Project
vingeldal updated the diff for D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Dec 30 2019, 7:07 AM · Restricted Project
vingeldal added a comment to D71963: clang-tidy doc: Add the severities description.

@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.

Dec 30 2019, 6:03 AM · Restricted Project

Dec 29 2019

vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Dec 29 2019, 3:01 AM · Restricted Project
vingeldal added a comment to D71963: clang-tidy doc: Add the severities description.

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.

Dec 29 2019, 2:57 AM · Restricted Project

Dec 28 2019

vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Dec 28 2019, 9:58 AM · Restricted Project
vingeldal updated the diff for D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Dec 28 2019, 9:53 AM · Restricted Project
vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Dec 28 2019, 8:06 AM · Restricted Project
vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Dec 28 2019, 7:38 AM · Restricted Project
vingeldal updated the diff for D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Dec 28 2019, 5:34 AM · Restricted Project
vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Dec 28 2019, 5:34 AM · Restricted Project

Dec 23 2019

vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Dec 23 2019, 12:36 PM · Restricted Project
vingeldal updated the diff for D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Dec 23 2019, 12:36 PM · Restricted Project

Dec 20 2019

vingeldal added inline comments to D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Dec 20 2019, 5:09 AM · Restricted Project
vingeldal updated the diff for D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.

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

Dec 20 2019, 5:09 AM · Restricted Project

Nov 14 2019

vingeldal created D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.
Nov 14 2019, 11:39 AM · Restricted Project