Page MenuHomePhabricator

vingeldal (Kim Viggedal)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 13 2019, 10:14 PM (19 w, 3 d)

Recent Activity

Mon, Mar 23

vingeldal added a comment to D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-arguments-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.
Mon, Mar 23, 12:31 AM · Restricted Project, Unknown Object (Project)

Fri, Mar 13

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?

Fri, Mar 13, 12:08 AM · Restricted Project

Thu, Mar 12

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++

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

Tue, Mar 10

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

Feb 24 2020

vingeldal added a comment to D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-arguments-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-arguments-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-arguments-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