Obsolete. There was no community consensus, or even a large enough discussion about this. What's more, the only dependee of this patch has superseded in ellegance, dropping the need for this group.

This patch will be replaced with the refactoring of the base check.

This patch will be replaced with the refactoring of the base check.

Refactored the check and rebased it against "current" master. This version now has the tests rewritten and fixed. In addition, this patch only introduces the very basic frame of the check (i.e. only strict type equality), nothing more. This is so that review can be done in a more digestible way.

I have re-analysed Bitcoin and Xerces with the new version (not yet published to Phab), and the results are looking really good! There is an insignificant change in the number of reports, 1 or 2 new ones compared to the previous, all explained by the fact that "unnamed parameters" are marked as ignored, i.e. the previous report (which reported 3 swappable parameters, one unnamed) is marked "Resolved" (1), but there is the (2 swappable one without the unnamed at the start/end) as "New". Due to this, I will spare you from showing the essentially same table from above again: the total number of results are unchanged, only the details of each (and they are changed in a positive (towards more sensible) direction).

In D69560#2487117, @aaron.ballman wrote:

In D69560#2487093, @whisperity wrote:

Not sure what CVR-modelling's default should be... it finds less when "off", but too easily silences crucial issues (such as memcpy(T*, const T*)).

My instinct is that if you accidentally swap a qualified parameter you'll get some other diagnostic about dropped qualifiers and so perhaps the default should be off, but perhaps there's something I'm not thinking of there.

I have posted two questions to GitHub, mostly related to the guideline rule and how free the implementation could be: #1732 and #1733, I think I tagged you on both.

I was just about to write an issue to the CoreGuidelines project, but I was @vingeldal has posted about "relatedness" before me: https://github.com/isocpp/CppCoreGuidelines/issues/1579. It was in the list of closed issues.

Ignore one-way implicit conversions

In D69560#2319340, @aaron.ballman wrote:

Congrats on the SCAM acceptance, I hope the presentation goes well!

But of course, after having written all of that, I forgot to upload the results themselves... 😲

Right, let's bump. I've checked the output of the checker on a set of test projects (our usual testbed, I should say) which contains around 15 projects, half of which was C and the other half C++. All projects were analysed independently in a virtual machine. All projects were analysed after checking out the latest release tag that was closest to Jan 2019. (I know this sounds dated, but the whole reproduction image was originally constructed for another checker I'm working on, and I did not wish to re-do the whole "What is needed to install this particular version?" pipeline.) The more important thing is that releases were analysed, which should, in theory, under-approximate the findings because releases are generally more polished than in-the-works code. The check was run as-is (sans a minor rebase issue that does not affect functionality) currently in the patch, namely, Diff 259508.

Actually, while the explanation is understandable for me with additional knowledge about the representation... I think it would be useful to add the most simple example from the iterator checkers to the end of the document, how this whole thing ties together and how it is useful in a checker.

In general, perhaps you should go over the rendered text once again and make use of emph and bold some more.
Explanation looks okay from my part, although I'm really not knowledgeable about CSA internals. But there are plenty of "typesetting" issues.

In D84176#2300488, @njames93 wrote:

Is this an artefact of how check-clang-tools also depends on all clang-tools-extra targets.

Ping. Can this go in? Still using this on a local fork, and still feels nice to be able to specify just a single tool.

I'd like to resurrect the discussion about this. Unfortunately, the concept of experimental- group (in D76545) has fallen silent, too. We will present the results of this analysis soon at IEEE SCAM (Source Code Analysis and Manipulation) 2020. While a previous submission about this topic was rejected on the grounds of not being in line with the conference's usual, hyper-formalised nature, with one reviewer especially praising the paper for its nice touch with the empirical world and the fact that neither argument swaps (another checker worked on by a colleague) nor the interactions of this issue with the type system (this checker) was really investigated in the literature for C++ before, we've tailored both the paper and the implementation based on reviewer comments from the scientific world, and the comments here.
The reviews were mostly favourable, excl. comments about the lack of formalisation and the unfortunate lack of modelling for template instantiations. Such a cold cut, however, only introduces false negatives into the result set. Which is fine, as the users will never see those non-existent reports!

Minor inline comments.

What about http://clang.llvm.org/doxygen/classclang_1_1CXXConversionDecl.html ?

One minor "typo", otherwise I think we are saying everything that could be and needs to be said. Also, @steakhal, that must have been one massive blame.

Some grammatical fixes and suggestions, inline. I might have absolutely butchered 80-col in the suggestions (thanks Phab for not showing any indication of line length...), so make sure you manually reformat the document before going forward!

In D81272#2218175, @aaron.ballman wrote:

While I agree with your observation about data and flow sensitivity, I approved on the belief that the check as-is provides enough utility to warrant adding it as-is. If someone wants to improve the check into being a CSA check, we can always deprecate this one at that point. However, if there are strong opinions that the check should start out as a CSA check because it requires that sensitivity for your needs, now's a good time to bring up those concerns.

In D81272#2218050, @aaron.ballman wrote:

Thanks to the new info, I think the check basically LGTM. Can you add some negative tests and documentation wording to make it clear that the check doesn't currently handle all logically equivalent predicates, like:

if (foo) {
} else {
  if (!foo) {
  }
}

// or
if (foo > 5) {
  if (foo > 3) {
  }
}

// or
if (foo > 5) {
  if (5 < foo) {
  }
}

(I'm assuming these cases aren't handled currently and that handling them isn't necessary to land the patch.)

In D72705#2210255, @balazske wrote:

More results in CodeChecker: emacs_errorreturn

@baloghadamsoftware Maybe there is a typo in the summary of the patch

In D84520#2206077, @balazske wrote:

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

What will happen with the ability to analyse a translation unit whose target was not part of LLVM_TARGETS_TO_BUILD of the current clang binary? Will it break, because the binary lacks the information on how to generate for the given target?

In D75229#2189666, @abelkocsis wrote:

I have managed to improve the checker, but could not set up the test files to work only in POSIX platforms. Could you help me or show me an example?

After using this for a while, it feels more natural and more obvious that these are the CTE targets, not the "clang/tools" test targets.

In D77150#2149870, @dkrupp wrote:

Since the analyzer cannot cannot model the size of the containers just yet (or precisely enough), what we are saying with this checker is to "always check the return value of the erase() function before use (for increment/decrement etc.) whether it is not past-end" .

Adam, are you sure that the user would like to enforce such a generic coding rule? Depending on the actual code analyzed, this would require this clumsy check at potentially too many places.

@lebedev.ri If you have an idea on who's competent in accepting this change, please update the reviewers field, I'm in the dark here.

In general, the test files should be cleaned up.

As a future work, when something support ifs, it should also support ?:, and eliminate redundant conditionals from there, too.

In D79330#2035850, @balazske wrote:

I was looking at CERT ARR32-C "Ensure size arguments for variable length arrays are in a valid range". The VLA should not have a size that is larger than std::numeric_limits<size_t>::max(), in other words "fit into a size_t value", or not?

Yes creating the too large VLA in itself is not a bug, only when sizeof is called on it because it can not return the correct size. A non-fatal error is a better option, or delay the check until the sizeof call. But probably the create of such a big array in itself is sign of code smell. The array actually does not need to be created to make the problem happen, only a sizeof call on a typedef-ed and too large VLA. (What does mean that "result of sizeof is implementation-defined"? Probably it can return not the size in bytes or "chars" but something other? In such a case the checker would be not correct.)

(Maybe this will make Phab not show "✅ Accepted"...)

Please check the summary of the patch, it seems to contain old information as well.

@steakhal You might want to update the patch summary before committing this to the upstream (it still mentions "not needing a visitor" 😄)

First things first, we were 50 thousand (!) patches behind reality. Rebased to master. Fixed it to compile, too. Otherwise, NFC so far.

Assuming direct control. Previous colleagues and university mates departed for snowier pastures, time to try to do something with this check.

• Re-organised code, removed debug prints, rebased, the usual tidy-up.
• Bug fixes on certain crashes like incomplete types, conversion operator templates, etc.
• Even more bug fixes against crashes, hopefully... sorry I lost the individual commits in a squash I think

• Better organisation of code, cleanup of debug prints, fix nomenclature of functions
• Explicitly mention the first and last param of the range for clarity
• Downlift the logic of joining and breaking ranges to main patch (this isn't terribly useful at this point, but both subsequent improvements to the check depend on this change)
  • Basically no longer assume that if param N can be swapped with param 1, then it can also be swapped with 2, 3, etc. without checking.
• Made the list of ignored parameter and type names configurable as a checker option
• Changed the default value of MinimumLength to 2, as prescribed in the guideline's text.

Adding @martong, he might have knowledge about the whole summary thing as he's tinkering the(?) STL checker.

@aaron.ballman @njames93 Should I write up a pitch mail to cfe-dev about this?

In D76545#1935603, @aaron.ballman wrote:

I think we want to keep the experimental checks grouped to their parent module rather than being in a module of their own.

In D76541#1935163, @njames93 wrote:

Forgive me if I'm wrong, but these kinds of changes typically don't require a review.

• Renamed check to experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.
• s/argument/parameter/g in the code and output where appropriate.