[clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[whisperity]

Summary

Finds function definitions where parameters of convertible types follow each other directly, making call sites prone to calling the function with swapped (or badly ordered) arguments.

Such constructs are usually the result of insufficient design and lack the exploitation of strong type capabilities that are possible in the language.
This check finds and flags function definitions, and not call sites. The reasoning is that while there are several ways to heuristically detect a potential swapped call, in the long term, ensuring a safe interface is more beneficial.

As an interface rule, it is understandable that it goes nuts over large projects that have yet to make their interfaces stricter and stronger.

Some of the most trivial flagged examples might be:

FILE *open(std::string_view Directory, std::string_view Filename, std::string_view Extension) { /* ... */ }

Originally, the check was meant to implement C++ Core Guideline rule I.24 "Avoid adjacent parameters of the same type when changing the argument order would change meaning" here, however, discussion about the rule's applicability is ongoing. For now, introducing the check outside the cppcoreguidelines- namespace so that people can still benefit from it.

[whisperity]

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 addition, I've implemented the "patterned name filtering" (a really rudimentary, but seemingly rather powerful version), with the rule "if two parameters are similar to at most 1 letter at the beginning or the end" (2), they should be scrapped from the result set.

Throwing out one-way implicit conversions (which I admit was a huge and utterly useless misunderstanding and misdesign from our part...) is also a good update. In addition, I've did the analysis with the explicit "do not report bool, bool, ... sequences" rule enabled. This silenced (as bool swaps are a more special problem wrt. "strong typing" - and even a separate guideline rule exists for them) 30 and 27 reports from the strictest case, for Bitcoing and Xerces, respectively.

There're still a few bugs (which I discovered but doesn't affect the above statements) to fix before I can split the patches and upload them to Phab.

---

(1): In CodeChecker, if you store a new result set over an already existing named result set, the bug reports that disappeared are marked "Resolved".
(2): lhs, rhs, text1, text2, text3, qmat, rmat, tmat, etc. are covered by this.

[whisperity]

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.

[whisperity]

• Added a fallback case to also diagnose when Clang parsed the two types to be canonically the same (we will elaborate a few cases of such equivalences later, such as typedefs)
• Fixed Clang-Format and Clang-Tidy warnings in the patch

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

58 ↗

(On Diff #319315)

Is there a need for the anonymous namespace? (We typically only use them when defining a class and try to make them as narrow in scope as possible, and use static for function declarations instead.)

67 ↗

(On Diff #319315)

Does using the fixed underlying type buy us something? As best I can tell, this will introduce a fair amount of implicit promotions to int anyway.

90–91 ↗

(On Diff #319315)

Heh, I don't know if we have any guidance on US English vs British English spellings. ;-)

105 ↗

(On Diff #319315)

176–177 ↗

(On Diff #319315)

Outside of field declarations, we typically only const-qualify pointer/reference types.

177 ↗

(On Diff #319315)

Some interesting test cases to consider: varargs functions and K&R C functions

196 ↗

(On Diff #319315)

Preferable to spell this type out to make it easier on someone reading the code to understand the type.

255 ↗

(On Diff #319315)

Hmm, one downside to using the printing policy to get the node name is that there can be alternative spellings for the same type that the user might reasonably expect to be applied. e.g., if the user expects to ignore bool datatypes and the printing policy wants to spell the type as _Bool, this won't behave as expected.

284 ↗

(On Diff #319315)

It might be good to move this to a more prominent location since it's a default value.

301 ↗

(On Diff #319315)

If we're going to include ForwardIt, we probably want things like RandomIt as well?

302 ↗

(On Diff #319315)

reverse_iterator and reverse_const_iterator too?

How about ranges?

402–403 ↗

(On Diff #319315)

Can you post a screenshot of what you mean?

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h

30 ↗

(On Diff #319315)

Do these data members need to be public?

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

I think this is a case where we could warn when the declaration is outside of a system header (perhaps as an option).

Thinking about it a bit more, declarations and definitions provide a novel way to get another kind of swap:

void func(int x, int y);
void func(int y, int x) {} // Oops, the swap was probably not intentional

which may or may not be interesting for a check (or its heuristics).

14 ↗

(On Diff #319315)

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

58 ↗

(On Diff #319315)

There will be two major parts, the modelling (and which is extended down the line with new models like implicit conversions) and the filtering (which throws away or breaks up results). Should I cut out the anonymous namespace? How about the inner namespaces, may they stay?

67 ↗

(On Diff #319315)

Will it? I thought if I use the proper LLVM bitmask/flag enum thing it will work properly. For now, it is good for the bit flag debug print, because it only prints 8 numbers, not 32.

90–91 ↗

(On Diff #319315)

Reflexes, my bad. (Similarly how I put the pointer * to the left side, where Stroustrup intended. At least my local pre-commit hooks take care of that.) Will try to remember this.

255 ↗

(On Diff #319315)

But then the diagnostic is inconsistent with what the compiler is configured to output as diagnostics, isn't it? Can I stringify through Clang with some "default" printing policy?

402–403 ↗

(On Diff #319315)

Given

Diag << SourceRange{First->getSourceRange().getBegin(), Last->getSourceRange().getEnd()};

The diagnostic is still produced with only a ^ at the original diag(Location), ignoring the fed SourceRange:

/home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
void redeclChain(int I, int J, int K) {}
                 ^

Instead of putting all the squigglies in as the range-location highlight, like how Sema can diagnose:

/home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
void redeclChain(int I, int J, int K) {}
                 ^~~~~~~~~~~~~~~~~~~

I would not want to put additional note tags in an otherwise already verbose output.

Back in 2019 I was investigating this issue specifically for another checker I was working on, but hit the wall... Somewhere deep inside where Tidy diagnostic stuff is translated and consumed to Clang stuff, it goes away. It shouldn't, because there are statements that seemingly copy the Diag.Ranges array over, but it still does not come out.

... EDIT: I found the relevant mailing list post.

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h

30 ↗

(On Diff #319315)

Yes. The modelling heuristics will have a bunch of recursive functions which all need the config variables (okay, not the MinimumLength one, but the other 4 or 5 that will be introduced) and giving them every time by copy would introduce its own adjacent swappable bool, bool parameter sequence issue.

The previous full version of the check which I'm incrementally reworking has signatures like:

static MixupData HowPossibleToMixUpAtCallSite(const QualType LType,
                                              const QualType RType,
                                              const ASTContext &Ctx,
                                              const bool CVRMixPossible,
                                              const bool ImplicitConversion,
                                              const bool IsUserDefinedConvertersOtherwisePossible);

static MixupData RefBindsToSameType(const LValueReferenceType *LRef,
                                    const Type *T, const ASTContext &Ctx,
                                    const bool IsRefRightType,
                                    const bool CVRMixPossible,
                                    const bool ImplicitConversionEnabled);

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

14 ↗

(On Diff #319315)

Good catch! This is what you get when you copy-paste. 😁

[whisperity]

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

I gave this some thought. It is a very good idea, but I believe not for this check, but D20689. What do you think of that? Maybe simply saying "call site v. function node that was called" could be extended with a completely analogous, string distance function based "function definition node v. redecl chain" case.

[whisperity]

• NFC Code style and organisation fixes

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

284 ↗

(On Diff #319315)

Moved it to the top.

302 ↗

(On Diff #319315)

How about ranges?

I would like to say that having an f(RangeTy, RangeTy) is exactly as problematic (especially if both are non-const) as having f(int, int) or f(void*, void*), and should be taken care of the same way (relatedness heuristic, name heuristic).
The idea behind ignoring iterator-ly named things was that "Iterators more often than not comes in pairs and you can't(*) do anything about it".

(*): Use ranges. Similarly how draw(int x, int y) is draw(Point2D) if we are enhancing type safety.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

58 ↗

(On Diff #319315)

The inner namespaces are totally fine. As for the anon namespace, I personally don't have a strong opinion against it, I brought it up mostly as a coding style guide nit, but I think removing it is helpful so that it's more obvious when a function has internal vs external linkage from its signature.

67 ↗

(On Diff #319315)

Ah, perhaps that's the case -- I'm not super familiar with the LLVM bitmask/flag macros. That said, if you think there's good utility for it, I'm happy to leave it as-is.

90–91 ↗

(On Diff #319315)

Heh, no worries, I'm betting if we did some code searches, we'd find plenty of British English spellings as well. :-D

255 ↗

(On Diff #319315)

The situation I'm worried about is something like this in C code:

#include <stdbool.h>

void func(bool b, bool c) {}

because the bool in the function signature will expand to _Bool after the preprocessor gets to it (because of the contents of <stdbool.h>). If the user writes bool in their configuration file because that's what they've written in their function signatures, will this fail because the printing policy says it should be _Bool?

Along similar lines, I wonder about things like struct S vs S (in C), signed char vs char, types within anonymous namespaces (where we print <anonymous namespace>), etc. Basically, any time when the printing policy may differ from what the user lexically writes in code.

302 ↗

(On Diff #319315)

I was thinking about ranges in situations where you also need them to come in pairs, like range union, range intersection, range difference, etc. Those cases feel like they suffer from the same problem as swap() where the interface is intentionally operating on two of the same types.

That said, I think paired range operations are *way* less common than paired iterator operations, so I think ranges should probably be left out.

402–403 ↗

(On Diff #319315)

... EDIT: I found the relevant mailing list post.

Oh wow, I had no idea! That's a rather unfortunate bug. :-(

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h

30 ↗

(On Diff #319315)

That's understandable logic, but then we're exposing essentially what amount to be implementation details. So it sounds like we're introducing a new code maintenance issue in order to solve another code maintenance issue. :-(

That said, I don't think leaving these as public is a deal-breaker.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

Hmmm, my impression is that this check is fundamentally about *parameter* ordering whereas D20689 is about *argument* ordering. Given that the example is about the ordering of parameters and doesn't have anything to do with call sites, I kind of think the functionality would be better in a parameter-oriented check.

That said, it doesn't feel entirely natural to add it to this check because this check is about parameters that are easily swappable *at call sites*, and this situation is not exactly that. However, it could probably be argued that it is appropriate to add it to this check given that swapped parameters between the declaration and the definition are likely to result in swapped arguments at call sites.

Don't feel obligated to add this functionality to this check (or invent a new check), though.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

255 ↗

(On Diff #319315)

Meanwhile, I realised we are talking of two entirely distinct things. I will look into how this PrintingPolicy stuff works... I agree that the ignore rules (where the comment was originally posted) should respect the text written into the code as-is. The diagnostics printing type names could continue using the proper printing policy (to be in line with how Sema diagnoses, AFAIK).

402–403 ↗

(On Diff #319315)

Speaking of bugs, shall I put it up on Bugzilla, or see if anyone reported already? Would require me digging into that "where are the data structures copied" kind of thing to figure it out again.

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h

30 ↗

(On Diff #319315)

These variables are const and "directly" created from the user-facing configuration arguments. But in the end once we are done with the implementation and everything is good to go and we see how many functions and what configuration variables are actually needed where, we can look into moving them into some internal struct again. But it feels rather unlikely.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

It just seems incredibly easier to put it into the other check, as that deals with names. But yes, then it would be a bit weird for the "suspicious call argument" check to say something about the definition... This check (and the relevant Core Guideline) was originally meant to consider only types (this is something I'll have to emphasise more in the research paper too!). Everything that considers names is only for making the check less noisy and make the actually emitted results more useful. However, I feel we should specifically not rely on the names and the logic too much.

But(!) your suggestion is otherwise a good idea. I am not sure if the previous research (with regards to names) consider the whole "forward declaration" situation, even where they did analysis for C projects, not just Java.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

402–403 ↗

(On Diff #319315)

Nevermind, this is worth a bug report just so we can track it in the proper location.

[steveire]

I haven't read through all the comments, but the word 'easily' implies 'desirable'. This check seems to be for finding params which are undesirably swappable, right?

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

402–403 ↗

(On Diff #319315)

If you don't mind doing that effort, it'd be appreciated! Even if you don't root cause the issue, having the report (or pinging an existing one) is still useful.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

However, I feel we should specifically not rely on the names and the logic too much.

I agree, to a point. I don't think the basic check logic should be name-sensitive, but I do think we need to rely on names to tweak the true/false positive/negative ratios. I think most of the time when we're relying on names should wind up being configuration options so that users can tune the algorithm to their needs.

We could put the logic into the suspicious call argument check with roughly the same logic -- the call site looks like it has potentially swapped arguments because the function redeclaration chain (which includes the function definition, as that's also a declaration) has inconsistent parameter names. So long as the diagnostic appears on the call site, and then refers back to the inconsistent declarations, it could probably work. However, I think it'd be a bit strange to diagnose the call site because the user of the API didn't really do anything wrong (and they may not even be able to change the declaration or the definition where the problem really lives).

[whisperity]

In D69560#2536570, @steveire wrote:

I haven't read through all the comments, but the word 'easily' implies 'desirable'. This check seems to be for finding params which are undesirably swappable, right?

The easily was to mean that the swap can be done with little effort (i.e. "in an easy fashion"?) and by accident. We want to find function signatures where that is the case, yes. TL;DR: "This function will be misused by a careless client, please try designing a better interface!"

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

402–403 ↗

(On Diff #319315)

And I managed to post a broken link originally... Here's the right one: https://bugs.llvm.org/show_bug.cgi?id=49000

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

But wait... suppose there is a project A, which sees the header for f(int x, int y) and has a call f(y, x). That will be diagnosed. But if project A is only a client of f, it should be highly unlikely that project A has the definition of f in their tree, right? So people of A will not get the warning for that. Only what is part of the compilation process will be part of the analysis process.

Similarly to how this check matches only function definitions, and never a prototype. The latter one would have bazillion of warnings from every header ever, with the user not having a chance to fix it anyways.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

But wait... suppose there is a project A

That's the point I was trying to make about it being strange to diagnose on the call site -- the user of the API didn't use it wrong, the designer of the API did something bad.

It's sounding more like this functionality is a good idea for a new check rather than an existing check.

[whisperity]

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

Okay, now I understand! But I did not intend to diagnose it on the call site. The other check itself would share the same configuration parameters and decision-making for string distance, and the "fwd decl vs. definition" diagnostic would come emitted to the definition's location! And the "call site vs. called function" would be diagnosed at the call site. And if there isn't a definition in the analysed TU for some function, the former logic is simply skipped.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

But I did not intend to diagnose it on the call site.

I'd find that to be a bit strange given that the other check is called readability-suspicious-call-argument, because there would be no call in that situation. Or am I misunderstanding? (Sorry if I'm being dense!)

[whisperity]

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

No, it is strange! And I think we will come up with a better idea for this because the check logic itself sounds very plausible, and needed. Either factor out the string distance logic somewhere and have two checks with separate matchers, but the same underlying logic then, or rename the other check... The two diagnostics would have a different message either way. The latter option (finding a new name for the check) sounds like the path of least resistance.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

10 ↗

(On Diff #319315)

Ah, yes! I think we're on the same page now. :-)

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

177 ↗

(On Diff #319315)

K&R C functions

Call me too young, but I had to look up what a "K&R C function" is, and I am absolutely baffled how this unholy construct is still supported! But: thanks to Clang supporting it properly in the AST, the checker works out of the box!

Given

int foo(a, b)
  int a;
  int b;
{
  return a + b;
}

We get the following output:

/tmp/knr.c:2:3: warning: 2 adjacent parameters of 'foo' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
  int a;
  ^
/tmp/knr.c:2:7: note: the first parameter in the range is 'a'
  int a;
      ^
/tmp/knr.c:3:7: note: the last parameter in the range is 'b'
  int b;
      ^

(even the locations are consistent!)

Should I add a test case for this? We could use a specifically C test case either way eventually...

---

varargs functions

This is a bit of terminology, but something tells me you meant the variadic function here, right? As opposed to type parameter packs.

Given

int sum(int ints...) {
  return 0;
}

the AST looks something like this:

`-FunctionDecl 0x56372e29e258 <variadic.cpp:1:1, line:3:1> line:1:5 sum 'int (int, ...)'
  |-ParmVarDecl 0x56372e29e188 <col:9, col:13> col:13 ints 'int'

Should we diagnose this? And if so, how? The variadic part is not represented (at least not at first glance?) in the AST. Understanding the insides of such a function would require either overapproximatic stuff and doing a looot of extra handling, or becoming flow sensitive... and we'd still need to understand all the va_ standard functions' semantics either way.

[riccibruno]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

337 ↗

(On Diff #319840)

I think that this will become unreachable after D84658 and D85033. I may be missing an argument, but in general I would argue that getNameForDiagnostic should never return an empty name for unnamed entities since this is not useful in diagnostics.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

177 ↗

(On Diff #319315)

Call me too young, but I had to look up what a "K&R C function" is, and I am absolutely baffled how this unholy construct is still supported!

Ah, to be innocent again, how I miss those days. :-D

Should I add a test case for this? We could use a specifically C test case either way eventually...

I think it'd be a useful case, but the one I was specifically more concerned with is:

// N.B.: this is C-specific and does not apply to C++.
void f();

int main(void) {
  f(1, 2, 3.4, "this is why we can't have nice things");
}

where the function has no prototype and so is treated as a varargs call.

This is a bit of terminology, but something tells me you meant the variadic function here, right? As opposed to type parameter packs.

Yes, sorry for being unclear, I am talking about variadic functions.

Should we diagnose this? And if so, how? The variadic part is not represented (at least not at first glance?) in the AST. Understanding the insides of such a function would require either overapproximatic stuff and doing a looot of extra handling, or becoming flow sensitive... and we'd still need to understand all the va_ standard functions' semantics either way.

Well, that's what I'm wondering, really. The arguments are certainly easy to swap because the type system can't help the user to identify swaps without further information (like format specifier strings). However, the checking code would be... highly unpleasant, I suspect. My intuition is to say that we don't support functions without prototypes at all (we just silently ignore them) and that we only check the typed parameters in a variadic function declaration (e.g., we'll diagnose void foo(int i, int j, ...); because of the sequential int parameters, but we won't diagnose void foo(int i, ...); even if call sites look like foo(1, 2);). WDYT?

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

177 ↗

(On Diff #319315)

It is definitely highly unpleasant, at one point I remember just glancing into the logic behind printf() related warnings in Sema and it was... odd, to say the least.

That is how the checker works right now. It will not diagnose int f() {} because from the looks of the function definition, it is a 0-parameter function. Same with the variadic ..., it is a single parameter function (which has a rather weird parameter type).

But yes, I think I'll make this explicit in the documentation (and FWIW, the research paper, too).

However, the checking code would be... highly unpleasant, I suspect.

Incredibly, because this check purposefully targets the function definition nodes only, like a cruise missile. For that logic, we would need to gather call sites for variadic functions and start doing some more magic with it.

FYI, templates are also handled in a way that only what is clear from definitions (and not instantiations... I think we can kind of call a particular "overload" call to a variadic like a template instantiation, just for the sake of the argument here) only:

template <typename T, typename U>
void f(T t1, T t2, U u) {} // [t1, t2] same type, WARN.

void g() { f(1, 2, 3); }} // NO-WARN: Even though "U" in this call is also "int", this is not fixed "easily enough" in the definition, so to prevent more false positives, we shut up.

template <>
void f(int i, int j, int k) {} // [i, k] WARN.

Anything that is only known through template instantiations is context-sensitive (the subsequent patch about typedef extends the tests and diagnostics about this), and thus, we forego trying to find the way.

template <typename T> struct vector { using value_type = T; };

template <typename T>
void f(T t, typename vector<T>::element_type u) {} // NO-WARN, dependent.

template <>
void f(int i, vector<int>::element_type i2) {} // WARN, can explicitly unfold the typedef and see "int == int".

template <typename T>
void g(typename vector<T>::element_type e1, typename vector<T>::element_type e2) {} // WARN, not dependent.

177 ↗

(On Diff #319315)

I definitely shall create and add a C file with test cases like this, marking them // NO-WARN, and explaining the reasoning. If for nothing else, maybe to know in the far future what can be improved upon.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

177 ↗

(On Diff #319315)

I definitely shall create and add a C file with test cases like this, marking them // NO-WARN, and explaining the reasoning. If for nothing else, maybe to know in the far future what can be improved upon.

Excellent, that's the solution I was hoping we'd come to! I find it's useful to add the C test for a function without a prototype (void f()) because some code that expects all function declarations to have a function prototype will assert or crash, so it helps shake out some bugs.

Also, I agree with your interpretation of the template case -- it's another intractable problem at this stage, so best to document how we handle it and move on.

[steveire]

In D69560#2536597, @whisperity wrote:

In D69560#2536570, @steveire wrote:

I haven't read through all the comments, but the word 'easily' implies 'desirable'. This check seems to be for finding params which are undesirably swappable, right?

The easily was to mean that the swap can be done with little effort (i.e. "in an easy fashion"?) and by accident.

I understand that. The problem is the name. The signal-to-kill-thread check is bad. The erase check is inaccurate. The roundings check is incorrect. The operator-in-strlen-in-alloc check is misplaced. Those are all words that indicate the negative. Your name indicates the positive ("easily"). The name indicates that the swappable params are desirable.

Why "easily" instead of "suspicious", "spuriously" or any of the other words that are already used?

[whisperity]

In D69560#2549959, @steveire wrote:

Why "easily" instead of "suspicious", "spuriously" or any of the other words that are already used?

The name was @aaron.ballman's idea after we postponed the experimental-cppcoreguidelines-yaddayaddayadda....
The best I could come up with is potentially. The suspicious is a problem because this check is an interface check, not a call-site check. (The sister check is currently called suspiciously-swapped-argument or something like that.)

The original check idea from C++ Core Guidelines also uses the phrase:

Reason: Adjacent arguments of the same type are easily swapped by mistake.

However, potentially-swappable-parameters just sounds... wonky. It does not carry the weight the rule otherwise meant to.
weakly-typed-parameters?

I am not good with names, sadly.

---

I am conflicted about saying that "easily" always refers to some "positive aspect", however. Use a [[unnamed trademarked safety device]] because you can end up easily cutting off your finger during work. Do not touch a stove/hot water fountain, because you can easily get burnt. Leaving Christmas candles unattended could easily lead to a house fire. Does anyone want to get hurt, burnt, their house destroyed, etc.? There is nothing "positive" in these suggestions or regulations either. And we are doing a tool that is practically the same thing, trying to prevent a (different kind of) fire.

[whisperity]

• Added a C test file with some C-specific language constructs that we may or may not match.
• Explained limitations with regards to variadics, templates, etc. better in the docs, and added test cases for these too.
• When comparing the type names against the ignored type name list, do not consider PrintingPolicy, but take exactly what's in the source code. This mostly concerns bool and _Bool at face value, so this is tested in the C file too.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

255 ↗

(On Diff #319315)

Right, @aaron.ballman I think I got this tested. Please check the C test file. bool becomes _Bool in C code by the preprocessor, but the printing policy still says both should be printed as bool. I added some test cases about this. It is weird, because without doing some extra magic specifically against macros (which might break detecting type names in other locations), I can't really test ignoring _Bool but not ignoring bool. Now, the diagnostics respect the printing policy, but the type names are taken with an empty printing policy, which should give us the result as-is in the code. Unfortunately, "as-is" still means _Bool for bool's case, so even if you ignore bool, you will still get results where bool is written, because bool is a macro (Tidy automatically puts a note about this!) and the real type is _Bool, as per the macro expansion.

Wow... okay, that explanation got convoluted.
Anyways, it's really weird. But: we no longer take the actual printing policy anymore, but the default, which should eliminate all these cases.
The bool/_Bool case doesn't fail because of the printing policy, it fails because bool is a macro in <stdbool.h>.

Also, the type name ignore list not a full match, but a substring match ignore list. So if you have a type named struct Foo, if you ignore Foo, both struct Foo, Foo, MyFoo, etc. will be also ignored. And it's case sensitive.

[whisperity]

Made detection of macros as "parameter type names" more robust.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

255 ↗

(On Diff #319315)

Nevermind, I think I figured this out... or at least managed to figure it out partially... This "SpellingLoc" and "ExpansionLoc" and all this stuff is really convoluted, and there doesn't seem to be any good documentation for them.

[whisperity]

NFC (With all the restructuring and trying to figure out how to implement the latest change, I managed to upload the wrong, unclean diff...)

[whisperity]

Make sure that overloadable binary operators are ignored properly.

[whisperity]

• [NFC] Reformat the documentation so option values are in single backticks.

[whisperity]

@aaron.ballman I've reworked and uploaded the final addition to this line, the crown jewel, the very reason this checker was developed (D75041). So, AFAIK, the "let's rewrite the whole thing with better code quality" work can be considered done, and we may move on with the review.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

54 ↗

(On Diff #326439)

const_reverse_iterator seems plausible here for the same reason as const_iterator.

56 ↗

(On Diff #326439)

Are you planning to remove the debugging code now that the check is approaching its final form?

363 ↗

(On Diff #326439)

Is this still needed in light of the comments from @riccibruno?

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

56 ↗

(On Diff #326439)

Actually, I would prefer not to. I removed every debug thing possible. However, and this is speaking from experience because I wrote this check two times already from basically scratch... the rest of the debug code that is part of the patch has to be there. If anything goes nuts, especially if there would be false positives later... it would be impossible to figure out what is going on during the modelling without seeing all the steps being taken.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

363 ↗

(On Diff #326439)

It doesn't look like to me as if those patches they commented were ever merged, but I'll check it out.

[aaron.ballman]

In general, I'm happy with this check (and the follow-ups), so I'm giving it my LG. However, it's complex enough that I think having some extra review from @alexfh, @hokein, or one of the other reviewers would be a good idea, so please hold off on landing it for a bit in case any of the other reviewers want to weigh in.

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

56 ↗

(On Diff #326439)

We don't often leave debugging statements in because they tend to cause a maintenance burden that doesn't justify their benefit. However, I agree with your observation that this code is quite difficult to reason through without debugging aids.

I don't insist on removing the code yet, but would say we should remain open to the idea if it causes a burden in practice. (Either in terms of keeping the debugging code up to date as the check evolves, but also in terms of compile time performance for the check if the debugging code paths turn out to be expensive on build times.)

[whisperity]

In D69560#2629555, @aaron.ballman wrote:

[...] so please hold off on landing it for a bit in case any of the other reviewers want to weigh in.

Due to how the patch itself only being useful in practice if all the pieces fall into place, I will definitely keep circling about until the entire patch tree is ✔. (Including the filtering heuristics, etc.)

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

56 ↗

(On Diff #326439)

All debugging code is wrapped into the LLVM_DEBUG macro specifically (that's why I even put this little "print bits" function behind NDEBUG) so they are not part of the builds.

I think in general if someone puts the effort into reading the code and has an interactive debugger they could figure it out (especially if they keep track of the recursion constantly), I tried to make everything nicely padded and all the variable names and control flow to make sense. (Of course the wealth of complexity comes in the follow-up patches.)

[aaron.ballman]

In D69560#2629616, @whisperity wrote:

In D69560#2629555, @aaron.ballman wrote:

[...] so please hold off on landing it for a bit in case any of the other reviewers want to weigh in.

Due to how the patch itself only being useful in practice if all the pieces fall into place, I will definitely keep circling about until the entire patch tree is ✔. (Including the filtering heuristics, etc.)

Good idea. :-)

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

56 ↗

(On Diff #326439)

All debugging code is wrapped into the LLVM_DEBUG macro specifically (that's why I even put this little "print bits" function behind NDEBUG) so they are not part of the builds.

They're part of debug builds still (which is the build configuration I tend to use most often). That said, I think it's fine to leave the debugging code in for now, especially as the check is being actively worked on.

[whisperity]

• Add const reverse iterator to the default-ignored typename list.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

363 ↗

(On Diff #326439)

I checked at they haven't been merged yet. Nonetheless, we can remove this logic later once that logic is in.

I added a test case for unnamed parameters with the current logic printing <unnamed>, so if the underlying call will return something like <unnamed parameter @ 24:48> we will know because that test will blow up.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

363 ↗

(On Diff #326439)

Thank you for checking and adding some test coverage for it!

[whisperity]

Rebased over D98635, allowing us to properly highlight the found "mixable adjacent parameter range" (assuming it's on the same line, of course):

[whisperity]

Made sure that the parameter's name is highlighted promptly in the "first|last parameter of the range is..." note.

[alexfh]

Thanks for the great work! This is mostly looking fine, but I've added a few comments. I could only make a quite superficial review so far, but I'll try to take a deeper look into this soon.

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

70 ↗

(On Diff #336622)

I think, this function is not needed (see the comment below), but if it is, a single std::string and in-place std::reverse() would be better, imo.

84–87 ↗

(On Diff #336622)

I suppose that textual representation of the flags would be easier to understand in debug logs than just bits. Maybe single characters or other abbreviations, if this needs to be compact.

113–120 ↗

(On Diff #336622)

I'd switch to scoped enums (enum class), remove the macro, and use simpler code like None = 0x1, Trivial = 0x2, Canonical = 0x4, etc.

133 ↗

(On Diff #336622)

What should this method do and how it should be used? Same for Mix::sanitize()

295–297 ↗

(On Diff #336622)

Would llvm::find(Check.IgnoredParameterNames, NodeName) != Check.IgnoredParameterNames.end() work?

302 ↗

(On Diff #336622)

No need for parentheses here.

313–320 ↗

(On Diff #336622)

Is there a chance that you're trying to reimplement a part of Lexer::makeFileCharRange functionality here?

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

129 ↗

(On Diff #336622)

Does the top-level const change anything with regard to the "mixability"? It's a purely implementation-side difference, callers could still swap parameters.

[alexfh]

BTW, any ideas why "patch application failed"? (See the Build status in the Diff Detail section)

It would be really nice to get pre-merge checks to work for the patch.

[whisperity]

In D69560#2747092, @alexfh wrote:

BTW, any ideas why "patch application failed"? (See the Build status in the Diff Detail section)

It would be really nice to get pre-merge checks to work for the patch.

Put simply, I think I haven't rebased the patch since a few changes ago. The past few changes were NFC/lint stuff only.

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

113–120 ↗

(On Diff #336622)

enum class is a good idea!

Unfortunately, I'm not really sold on the hexa literals... It makes the code confusing the read, because you have to keep in mind which range the hexa literals mean, and when there is a shift in position, e.g. if you read 0x20 you have to count out that okay, that's 2nd position, so at least 16, so at least the 4th bit, but it is two, so you shift it again to the right, so in the bit pattern it turns on the 5th bit actually.
It makes it very easy to misread or misinterpret things, especially when trying to work on the source code.
(In the last extension patch, the number of elements in the bit mask go up to the 8th or the 9th bit, I don't remember exactly at the top of my head.)

Compared to this, the current text when read immediately tells you which "decimal" position you should expect the appropriate bit to be toggled on, if the flag is there.

Binary literals, on the other hand, would be amazing to use here, sadly we're only in C++14.

133 ↗

(On Diff #336622)

Once there are more flags in the bitmask (not just "None" and "Trivial"), this method deals with sanitising the flags that are toggled on and off by the recursive descent on the type tree.

For example, if there is a typedef involved (see D95736), it will toggle on the typedef flag, and start investigating the aliased type. This subsequent investigation might figure out that the aliased type is trivial, or it is distinct. In case it is distinct, that level of the recursion will turn on the "Not mixable" flag. In this case, when the investigation returns completely, we will have "Typedef" and "None" both turned on. This method will throw away everything but the "None" bit, so when the data is returned to the diagnostic-generating part of the check, it will know what to do.

313–320 ↗

(On Diff #336622)

Does that expand the token length properly? I took this idiom of getLocForEndOfToken from various other Tidy checks... basically, anything that deals with replacing some sort of textual range in the code calls to this function. I need to have the exact ending location so I can obtain the full text, as appears in the source file. In this case, to compare against the user's input.

E.g. here is one example from utils/UsingInserter.cpp:

  32   │ Optional<FixItHint> UsingInserter::createUsingDeclaration(
  33   │     ASTContext &Context, const Stmt &Statement, StringRef QualifiedName) {
/* ... */
  42   │   SourceLocation InsertLoc = Lexer::getLocForEndOfToken(
  43   │       Function->getBody()->getBeginLoc(), 0, SourceMgr, Context.getLangOpts());

A quick grep for makeCharRange in the repository didn't turn up any significant result, apart from one unit test.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

129 ↗

(On Diff #336622)

Unfortunately, coding styles differ whether people mark local variables const or not, across projects. Even with pointers: in some cases, a swapped call to memcpy might be caught if the user made one of the pointers passed already const void* instead of just void*.

D95736 deals with implementing the logic for this, it is exposed as a user-configurable option whether they want to consider const stuff the same as non-const stuff.

[whisperity]

There was a bit of talk on Twitter recently about strong typing which reminded me of this checker! 😁 I'll fix all issues in one go once we're through with all the comments, I'd like to see the full picture of what needs to be done.

[whisperity]

All changes are NFC and styling only.

• Remove the FB macro in favour of explicitly specifying the bit flag value
• Change debug printout from bit pattern to individual significant letters

[whisperity]

In D69560#2747092, @alexfh wrote:

BTW, any ideas why "patch application failed"? (See the Build status in the Diff Detail section)

I think I figured out why the pre-build CI isn't working... it seems that the fact that this patch depends on an abandoned revision D76545 misleads the CI logic as it sees the dependency, and even though it is abandoned, tries to download and apply the contents of D76545, which makes it fail...

INFO    D76545#251840 commit 1e9b6b89a7b5c49612018b120c2c142106056f82 has a later commit date then7cdbf1ed4b94259a3aad2a7575e928fa61b0e57e
[...]
ERROR   error: patch failed: clang-tools-extra/docs/clang-tidy/index.rst:62
error: clang-tools-extra/docs/clang-tidy/index.rst: patch does not apply
error: patch failed: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h:15
error: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h: patch does not apply

[whisperity]

(Try nudging the CI to only apply this commit after removal of parent links.)

[whisperity]

Attempt to fix build failure on Windows buildbot (operator new was taking unsigned long instead of size_t in a test which resulted in hard error).

[aaron.ballman]

This is a very involved check that I think is going to produce some interesting results for users, thank you for working so diligently on it! I think all of the patches in the series have now been accepted and this is ready to land. @whisperity, are you aware of any changes that are left to be made?

[whisperity]

In D69560#2840554, @aaron.ballman wrote:

This is a very involved check that I think is going to produce some interesting results for users, thank you for working so diligently on it! I think all of the patches in the series have now been accepted and this is ready to land. @whisperity, are you aware of any changes that are left to be made?

I'll ping @martong privately if they have additional comments about D75041, but otherwise, no, I think no more things left to do. Landing checklist complete.

[aaron.ballman]

In D69560#2840579, @whisperity wrote:

In D69560#2840554, @aaron.ballman wrote:

This is a very involved check that I think is going to produce some interesting results for users, thank you for working so diligently on it! I think all of the patches in the series have now been accepted and this is ready to land. @whisperity, are you aware of any changes that are left to be made?

I'll ping @martong privately if they have additional comments about D75041, but otherwise, no, I think no more things left to do. Landing checklist complete.

Great! Unless you hear from @alexfh, @martong, or someone else with additional concerns, I think you're set to land this!

[alexfh]

I thought I sent this batch of comments loong ago =\

Better late than never.

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

113–120 ↗

(On Diff #336622)

First of all, do we ever need to know the position of the set bit in these flag enumerators? Isn't it enough that each enumerator has a distinct bit set? If so, there's not much complexity in reading and expanding sequences like 0x0001, 0x0002, 0x0004, 0x0008, 0x0010, 0x0020, 0x0040, 0x0080, 0x0100, 0x0200, ....
However, if that's not to your taste, there's another frequently used style of defining flag enumerations in LLVM: 1 << 0, 1 << 1, 1 << 2, 1 << 3, ....

Either of these is fine, but using macros for this purpose is not necessary and doesn't make the code any easier to read or to modify.

Compared to this, the current text when read immediately tells you which "decimal" position you should expect the appropriate bit to be toggled on, if the flag is there.

Do I understand correctly that you need this exclusively for the purpose of reading debug logs? If so, it seems to be better to decode the bits before logging them. See the comment on line 87 above.

133 ↗

(On Diff #336622)

Thanks for the explanation! But it would be very helpful to have this information in the code, not only in a code review comment.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

129 ↗

(On Diff #336622)

The const in const void * is not top-level, it applies to the pointed at type. A top-level const here would be const void * const (a constant pointer to a constant void) or void * const - a const pointer to a non-const void. Now getting back to the original question. Consider this function:

void qualified1(int I, const int CI) {}

The top-level const for the second parameter isn't a part of the function signature and doesn't help with prevention of the unintentionally swapped parameters. I guess, the same goes for top-level volatile.

Whatever arguments x and y are passed to qualified1 they can be swapped without being noticed.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

113–120 ↗

(On Diff #336622)

Yes, there individual positions themselves don't matter! The only thing that matters is that some flags are < than None and the rest are >=, but that's only needed because of the ugly workaround.

I turned the debug printouts to use a manual formatting with "reasonable" characters being set (e.g. T for Trivial or & for reference). It's much more readable and straight-forward, because you actually do not need to know the position of the bits themselves, those don't matter.

133 ↗

(On Diff #336622)

The full patch stack landed, and this function has a much more elaborated implementation (with more comments), I believe. In this patch, there was indeed nothing to add here.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

129 ↗

(On Diff #336622)

True. We can think about this later. The modelling logic would be made even more complicated by handling this, however, and the reports themselves would become asymmetric, because to understand the reports, users would need to actively think about this particular example (whether something is a "top-level const" or a "non-top-level const").

I know the check has almost been made into a CppCG checker (which in the strictest case says to only warn if something has the same type) and then we backtracked from it, some of these design decisions are hereditary from the previous reviews and development process.

I think the fact that there is a useful way from the user's perspective to toggle this is beneficial for two reasons:

• By default, the number of warnings are smaller. To an already verbose checker as it is, this is a clear upside.
• Even if mathematically there is no difference between I and CI from a language standpoint, if the const is there, it could be indicative of the user meaning something (similar to how I tried indicating a few things by making local variables const, and then during the review of the patch, I was told to not do that. It seems Tidy uses this coding style (something I do not personally agree with, local const variables prevent me from mistakes too!), where there are other project that use a different style). So in essence, this still allows us to respect coding styles in some way. By making one of the variables const, you wanted to indicate something - something that the checker can pick up on, and give you the choice to not start yelling immediately. At least when you're using the default settings and not tuning the individual options.

[RKSimon]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

57 ↗

(On Diff #354803)

@whisperity There is a missing comma at the end of the "const_reverse_iterator" line causing implicit concatenation with the following "Constreverseiterator" line.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

57 ↗

(On Diff #354803)

Thanks... I'll get around to hotfixing this ASAP, and hope it can be backported to 13.0...

(I wish there was a checker for this? 🙄)

clang-tools-extra/clang-tidy/experimental/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyExperimentalModule
+  CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp
   ExperimentalTidyModule.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h

+//== CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h clang-tidy =//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPERIMENTAL_CPPCOREGUIDELINESAVOIDADJACENTPARAMETERSOFTHESAMETYPECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPERIMENTAL_CPPCOREGUIDELINESAVOIDADJACENTPARAMETERSOFTHESAMETYPECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace experimental {
+
+/// Finds function definitions where parameters of the same type follow each
+/// other directly, making call sites prone to calling the function with swapped
+/// or badly ordered arguments.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.html
+class CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck : public ClangTidyCheck {
+public:
+  CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck(
+      StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+  /// Returns whether the given parameter is ignored based on its type name
+  /// or parameter name.
+  bool isIgnored(const ParmVarDecl *Node) const;
+
+  /// The minimum length of an adjacent range required to have to produce
+  /// a diagnostic.
+  const unsigned MinimumLength;
+
+  /// The parameter names (as written in the code) to be ignored when
+  /// checking for adjacent parameter ranges.
+  const std::vector<std::string> IgnoredParamNames;
+
+  /// The parameter type name suffixes (as written in the code) to be ignored
+  /// when checking for adjacent parameter ranges.
+  const std::vector<std::string> IgnoredParamTypes;
+
+  /// Whether to consider 'T' and 'const T'/'volatile T'/etc. arguments to be
+  /// possible mixup.
+  const bool CVRMixPossible;
+};
+
+} // namespace experimental
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPERIMENTAL_CPPCOREGUIDELINESAVOIDADJACENTPARAMETERSOFTHESAMETYPECHECK_H

clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp

+// CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp  clang-tidy //
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/BitmaskEnum.h"
+#include "llvm/ADT/SmallVector.h"
+
+#include <string>
+#include <utility>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace experimental {
+
+namespace {
+
+/// Annotates a possible mix of parameters based on which language construct
+/// is responsible for allowing thee mix to happen. This is a bitflag.
+enum MixupTag : unsigned char {
+  MIXUP_Invalid = 0, //< Sentinel 0 bit pattern value for masking. DO NOT USE!
+
+// Set the bit at index N to 1 as the enum constant. N = 0 is invalid.
+#define BIT(Name, N) MIXUP_##Name = (1ull << (N##ull - 1ull))
+  BIT(None, 1),    //< Mixup is not possible.
+  BIT(Trivial, 2), //< No extra information needed.
+  BIT(Typedef, 3), //< Parameter of a typedef which resolves to an effective
+                   //< desugared type same as the other arg.
+  BIT(RefBind, 4), //< Parameter mixes with another due to reference binding.
+  BIT(CVR, 5),     //< Parameter mixes with another through implicit
+                   //< qualification.
+#undef BIT
+
+  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ MIXUP_CVR)
+};
+LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
+
+/// A named tuple that contains which parameter with which other parameter
+/// can be mixed up in what fashion.
+struct Mixup {
+  const ParmVarDecl *First, *Second;
+  MixupTag Flags;
+
+  Mixup(const ParmVarDecl *A, const ParmVarDecl *B, MixupTag Flags)
+      : First(A), Second(B), Flags(Flags) {}
+
+  Mixup operator|(MixupTag EnableFlags) const {
+    return {First, Second, Flags | EnableFlags};
+  }
+  Mixup &operator|=(MixupTag EnableFlags) {
+    Flags |= EnableFlags;
+    return *this;
+  }
+
+  /// Sanitises the Mixup's flags so it doesn't contain contradictory bits.
+  void sanitise() {
+    assert(Flags != MIXUP_Invalid &&
+           "Mixup tag had full zero bit pattern value!");
+
+    if (Flags & MIXUP_None) {
+      // If at any point the checks mark the mixup impossible, it is just simply
+      // impossible.
+      Flags = MIXUP_None;
+      return;
+    }
+
+    if (Flags == MIXUP_Trivial)
+      return;
+
+    if (Flags ^ MIXUP_Trivial)
+      // If any other bits than Trivial is set, unset Trivial, so only the
+      // annotation bits warranting extra diagnostic are set.
+      Flags &= ~MIXUP_Trivial;
+  }
+};
+
+static_assert(std::is_trivially_copyable<Mixup>::value,
+              "keep Mixup trivially copyable!");
+
+/// Represents a (closed) range of adjacent parameters that can be mixed up at
+/// a call site.
+struct MixableAdjacentParamRange {
+  std::size_t NumParamsChecked = 0;
+  llvm::SmallVector<Mixup, 8> Mixups;
+
+  const ParmVarDecl *getFirstParm() const {
+    // The first element added to the mixup vector has the left hand
+    // side set to the first argument in the range, if any is added.
+    assert(!Mixups.empty());
+    return Mixups.front().First;
+  }
+
+  const ParmVarDecl *getLastParm() const {
+    // There builder function paramEqualTypeRange breaks building an instance of
+    // this type if it finds something that can not be mixed up, by going
+    // *forward* in the list of arguments. So at the moment of break, the right
+    // hand side of the last conversion in the vector is the last argument in
+    // the adjacency range.
+    assert(!Mixups.empty());
+    return Mixups.back().Second;
+  }
+};
+
+} // namespace
+
+/// Returns whether an lvalue reference refers to the same type as T.
+static MixupTag RefBindsToSameType(const LValueReferenceType *LRef,
+                                   const Type *T, bool CVRMixPossible);
+
+/// Returns whether LType and RType refer to the same type in a sense that at a
+/// call site it is possible to mix the types up if the actual arguments are
+/// specified in opposite order.
+/// \returns MixupTag indicating how a mixup between the arguments happens.
+/// The final output of this potentially recursive function must be sanitised.
+static MixupTag HowPossibleToMixUpAtCallSite(const QualType LType,
+                                             const QualType RType,
+                                             const ASTContext &Ctx,
+                                             const bool CVRMixPossible) {
+  if (LType == RType)
+    return MIXUP_Trivial;
+
+  // Remove certain sugars that don't affect mixability from the types.
+  if (dyn_cast<const ParenType>(LType.getTypePtr()))
+    return HowPossibleToMixUpAtCallSite(LType.getSingleStepDesugaredType(Ctx),
+                                        RType, Ctx, CVRMixPossible);
+  if (dyn_cast<const ParenType>(RType.getTypePtr()))
+    return HowPossibleToMixUpAtCallSite(
+        LType, RType.getSingleStepDesugaredType(Ctx), Ctx, CVRMixPossible);
+
+  // An argument of type 'T' and 'const T &' may bind with the same power.
+  // (Note this is a different case, as 'const T &' is a '&' on the top level,
+  // and only then a const.)
+  if (LType->isLValueReferenceType() || RType->isLValueReferenceType()) {
+    // (If both is the same reference type, earlier a return happened.)
+
+    if (LType->isLValueReferenceType()) {
+      MixupTag RefBind = RefBindsToSameType(LType->getAs<LValueReferenceType>(),
+                                            RType.getTypePtr(), CVRMixPossible);
+      // RefBind may or may not have given us a tag (e.g. reference was to a
+      // typedef) via a recursive chain back to this function. Apply the
+      // "bind power" tag here to indicate a reference binding happened.
+      // (If RefBind was MIXUP_None, a later sanitise step will undo every bit
+      // except for None.)
+      return RefBind | MIXUP_RefBind;
+    }
+    if (RType->isLValueReferenceType()) {
+      MixupTag RefBind = RefBindsToSameType(RType->getAs<LValueReferenceType>(),
+                                            LType.getTypePtr(), CVRMixPossible);
+      return RefBind | MIXUP_RefBind;
+    }
+  }
+
+  // A parameter of type 'T' and 'const T' may bind with the same power.
+  // Case for both types being const qualified (for the same type) is handled
+  // by LType == RType.
+  if (LType.getLocalCVRQualifiers() || RType.getLocalCVRQualifiers()) {
+    if (!CVRMixPossible)
+      return MIXUP_None;
+
+    return HowPossibleToMixUpAtCallSite(LType.getUnqualifiedType(),
+                                        RType.getUnqualifiedType(), Ctx,
+                                        CVRMixPossible) |
+           MIXUP_CVR;
+  }
+
+  {
+    const auto *LTypedef = LType->getAs<TypedefType>();
+    const auto *RTypedef = RType->getAs<TypedefType>();
+    if (LTypedef && RTypedef)
+      return MIXUP_Typedef | HowPossibleToMixUpAtCallSite(LTypedef->desugar(),
+                                                          RTypedef->desugar(),
+                                                          Ctx, CVRMixPossible);
+    if (LTypedef)
+      return MIXUP_Typedef |
+             HowPossibleToMixUpAtCallSite(LTypedef->desugar(), RType, Ctx,
+                                          CVRMixPossible);
+    if (RTypedef)
+      return MIXUP_Typedef |
+             HowPossibleToMixUpAtCallSite(LType, RTypedef->desugar(), Ctx,
+                                          CVRMixPossible);
+  }
+
+  if (LType->isPointerType() && RType->isPointerType())
+    // (Both types being the exact same pointer is handled by LType == RType.)
+    return HowPossibleToMixUpAtCallSite(
+        LType->getPointeeType(), RType->getPointeeType(), Ctx, CVRMixPossible);
+
+  // A parameter of type 'T' and 'const T' may bind with the same power.
+  // Case for both types being const qualified (for the same type) is handled
+  // by LType == RType.
+  if (CVRMixPossible &&
+      (LType.isLocalConstQualified() || LType.isLocalVolatileQualified()))
+    return MIXUP_CVR | HowPossibleToMixUpAtCallSite(LType.getUnqualifiedType(),
+                                                    RType, Ctx, CVRMixPossible);
+  if (CVRMixPossible &&
+      (RType.isLocalConstQualified() || RType.isLocalVolatileQualified()))
+    return MIXUP_CVR |
+           HowPossibleToMixUpAtCallSite(LType, RType.getUnqualifiedType(), Ctx,
+                                        CVRMixPossible);
+
+  return MIXUP_None;
+}
+
+static MixupTag RefBindsToSameType(const LValueReferenceType *LRef,
+                                   const Type *T, bool CVRMixPossible) {
+  const QualType ReferredType = LRef->getPointeeType();
+  if (!ReferredType.isLocalConstQualified())
+    // A non-const reference doesn't bind with the same power as a "normal"
+    // by-value parameter.
+    return MIXUP_None;
+
+  if (const auto *TypedefTy = ReferredType.getTypePtr()->getAs<TypedefType>())
+    // If the referred type is a typedef, try checking the mixup-chance on the
+    // desugared type.
+    return HowPossibleToMixUpAtCallSite(TypedefTy->desugar(), QualType{T, 0},
+                                        TypedefTy->getDecl()->getASTContext(),
+                                        CVRMixPossible);
+
+  return ReferredType.getTypePtr() == T ? MIXUP_Trivial : MIXUP_None;
+}
+
+/// Gets the mixable range of the parameters of F starting with the param at
+/// index StartIdx.
+static MixableAdjacentParamRange BuildMixableParameterRange(
+    const CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck &Checker,
+    const FunctionDecl *F, unsigned int StartIdx) {
+  MixableAdjacentParamRange MixRange;
+  const unsigned int ParamCount = F->getNumParams();
+  assert(StartIdx < ParamCount && "invalid start index given!");
+  const ASTContext &Ctx = F->getASTContext();
+  const ParmVarDecl *First = F->getParamDecl(StartIdx);
+
+  // A parameter (the one at StartIdx) was checked.
+  MixRange.NumParamsChecked = 1;
+
+  // Try checking parameters of the function from StartIdx until the range
+  // breaks. The range contains parameters that are mutually mixable with each
+  // other.
+  for (unsigned int I = StartIdx + 1; I < ParamCount; ++I) {
+    const ParmVarDecl *Ith = F->getParamDecl(I);
+    if (Checker.isIgnored(Ith))
+      // If the next parameter in the range is ignored, break the range.
+      break;
+
+    bool AnyMixupStored = false;
+    for (unsigned int J = StartIdx; J < I; ++J) {
+      const ParmVarDecl *Jth = F->getParamDecl(J);
+      Mixup M{Jth, Ith,
+              HowPossibleToMixUpAtCallSite(Jth->getType(), Ith->getType(), Ctx,
+                                           Checker.CVRMixPossible)};
+      M.sanitise();
+      assert(M.Flags != MIXUP_Invalid &&
+             "Bits fell off, result is sentinel value.");
+
+      if (M.Flags != MIXUP_None) {
+        MixRange.Mixups.emplace_back(M);
+        AnyMixupStored = true;
+      }
+    }
+
+    if (!AnyMixupStored)
+      // If there is no "new" mixup possibility for the Ith param, it signifies
+      // the end of range.
+      break;
+
+    // If the loop did not break earlier, another parameter was found to be
+    // mixable.
+    ++MixRange.NumParamsChecked;
+  }
+
+  return MixRange;
+}
+
+/// Prints the type's textual representation to the output stream. This printer
+/// discards as many sugar as it can, for example removing typedefs and printing
+/// the underlying type.
+static void PutTypeName(const QualType QT, llvm::raw_ostream &OS,
+                        const PrintingPolicy &PP) {
+  SplitQualType SQT = QT.split();
+  const Type *Ty = SQT.Ty;
+
+  if (const auto *DecayTy = dyn_cast<DecayedType>(Ty))
+    return PutTypeName(DecayTy->getPointeeType(), OS, PP);
+
+  if (const auto *PointerTy = dyn_cast<PointerType>(Ty)) {
+    PutTypeName(PointerTy->getPointeeType(), OS, PP);
+    // Note: this might print types with weird grammar (function pointers, etc.)
+    // in a weird way.
+    OS << " *";
+
+    // Qualifications of a pointer has to be after the '*'.
+    // (The member is qualified in a subsequent recursive call.)
+    if (SQT.Quals.hasConst())
+      OS << " const";
+    if (SQT.Quals.hasVolatile())
+      OS << " volatile";
+    if (SQT.Quals.hasRestrict())
+      OS << " restrict";
+    return;
+  }
+
+  // Qualifications of everything else can be written at the front.
+  if (SQT.Quals.hasConst())
+    OS << "const ";
+  if (SQT.Quals.hasVolatile())
+    OS << "volatile ";
+  if (SQT.Quals.hasRestrict())
+    OS << "restrict ";
+
+  if (const auto *RefTy = dyn_cast<ReferenceType>(Ty)) {
+    PutTypeName(RefTy->getPointeeType(), OS, PP);
+    if (RefTy->isLValueReferenceType())
+      OS << " &";
+    else if (RefTy->isRValueReferenceType())
+      OS << " &&";
+  } else if (const auto *BuiltinTy = dyn_cast<BuiltinType>(Ty))
+    OS << BuiltinTy->getName(PP);
+  else if (const TagDecl *TagDeclTy = Ty->getAsTagDecl()) {
+    std::string Name = TagDeclTy->getName().str();
+    if (!Name.empty() && !TagDeclTy->getASTContext().getLangOpts().CPlusPlus)
+      // Prepend "struct" before "T" in C mode.
+      Name = TagDeclTy->getKindName().str().append(" ").append(Name);
+
+    if (Name.empty())
+      Name = TagDeclTy->getTypedefNameForAnonDecl()->getName().str();
+
+    if (!Name.empty())
+      OS << Name;
+    else
+      OS << "(unknown "
+         << static_cast<const DeclContext *>(TagDeclTy)->getDeclKindName()
+         << ")";
+  } else if (const auto *TypedefTy = dyn_cast<TypedefType>(Ty))
+    PutTypeName(TypedefTy->desugar(), OS, PP);
+  else if (const auto *TemplateTy = dyn_cast<TemplateTypeParmType>(Ty))
+    OS << TemplateTy->getDecl()->getName();
+  else if (const auto *DependentNTy = dyn_cast<DependentNameType>(Ty))
+    OS << DependentNTy->getIdentifier()->getName();
+  else if (const auto *FunctionPtrTy = dyn_cast<FunctionProtoType>(Ty)) {
+    PutTypeName(FunctionPtrTy->getReturnType(), OS, PP);
+    OS << " (";
+    for (unsigned Idx = 0; Idx < FunctionPtrTy->getNumParams(); ++Idx) {
+      PutTypeName(FunctionPtrTy->getParamType(Idx), OS, PP);
+      if (Idx < FunctionPtrTy->getNumParams() - 1)
+        OS << ", ";
+    }
+    OS << ')';
+  } else if (const auto *SpecialisationTy =
+                 dyn_cast<TemplateSpecializationType>(Ty)) {
+    SpecialisationTy->getTemplateName().print(OS, PP, /* SuppressNNS =*/true);
+    OS << '<';
+    for (const TemplateArgument &Arg : SpecialisationTy->template_arguments()) {
+      Arg.print(PP, OS);
+    }
+    OS << '>';
+  } else if (const auto *ParenTy = dyn_cast<ParenType>(Ty))
+    PutTypeName(ParenTy->getInnerType(), OS, PP);
+  else if (const auto *PackTy = dyn_cast<PackExpansionType>(Ty)) {
+    PutTypeName(PackTy->getPattern(), OS, PP);
+    OS << "...";
+  } else
+    // There are things like "GCC Vector type" and such that who knows how
+    // to print properly?
+    OS << "<unhandled Type of " << Ty->getTypeClassName() << '>';
+}
+
+static std::string TypeNameAsString(const QualType QT,
+                                    const PrintingPolicy &PP) {
+  std::string Ret;
+  {
+    llvm::raw_string_ostream OS{Ret};
+    PutTypeName(QT, OS, PP);
+  }
+
+  if (Ret.empty())
+    Ret = "<unprintable type?>";
+  return Ret;
+}
+
+static const std::string DefaultIgnoredParamNames =
+    utils::options::serializeStringList({"iterator", "Iterator", "begin",
+                                         "Begin", "end", "End", "first",
+                                         "First", "last", "Last"});
+
+static const std::string DefaultIgnoredParamTypes =
+    utils::options::serializeStringList(
+        {"it", "It", "iterator", "Iterator", "inputit", "InputIt", "forwardit",
+         "ForwardIt", "bidirit", "BidirIt", "constiterator", "const_iterator",
+         "Const_Iterator", "Constiterator", "ConstIterator"});
+
+bool CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck::isIgnored(
+    const ParmVarDecl *Node) const {
+  if (!Node->getIdentifier())
+    return false;
+
+  StringRef NodeName = Node->getName();
+  if (llvm::any_of(IgnoredParamNames,
+                   [&NodeName](const std::string &IgnoredName) {
+                     return NodeName == IgnoredName;
+                   }))
+    return true;
+
+  const Type *NodeType =
+      Node->getType().getDesugaredType(Node->getASTContext()).getTypePtr();
+  assert(NodeType && "parameter without a type?");
+
+  std::string NodeTypeName = Node->getType().getAsString();
+  if (!NodeTypeName.empty()) {
+    if (llvm::any_of(IgnoredParamTypes, [&NodeName, &NodeTypeName](
+                                            const std::string &IgnoredName) {
+          if (NodeTypeName == IgnoredName)
+            return true;
+
+          std::size_t IgnoredNameLen = IgnoredName.length();
+          if (NodeTypeName.length() > IgnoredNameLen &&
+              NodeTypeName.compare(NodeTypeName.length() - IgnoredNameLen,
+                                   IgnoredNameLen, IgnoredName) == 0)
+            return true;
+
+          return false;
+        }))
+      return true;
+  }
+
+  return false;
+}
+
+AST_MATCHER_P(FunctionDecl, hasAtLeastNumParams, unsigned int, N) {
+  return Node.getNumParams() >= N;
+}
+
+static constexpr unsigned DefaultMinLength = 2;
+
+/// A range length of 0 or 1 is useless as it would simply report for every
+/// parameter. Fix the user's input if bad value is given.
+static unsigned ClampMinimumLength(const unsigned Option) {
+  return Option < 2 ? DefaultMinLength : Option;
+}
+
+CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck::
+    CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck(
+        StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      MinimumLength(
+          ClampMinimumLength(Options.get("MinimumLength", DefaultMinLength))),
+      IgnoredParamNames(utils::options::parseStringList(
+          Options.get("IgnoredNames", DefaultIgnoredParamNames))),
+      IgnoredParamTypes(utils::options::parseStringList(
+          Options.get("IgnoredTypes", DefaultIgnoredParamTypes))),
+      CVRMixPossible(Options.get("CVRMixPossible", false)) {}
+
+void CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "MinimumLength", MinimumLength);
+  Options.store(Opts, "IgnoredNames",
+                utils::options::serializeStringList(IgnoredParamNames));
+  Options.store(Opts, "IgnoredTypes",
+                utils::options::serializeStringList(IgnoredParamTypes));
+  Options.store(Opts, "CVRMixPossible", CVRMixPossible);
+}
+
+void CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck::
+    registerMatchers(MatchFinder *Finder) {
+  const LangOptions &Opts = getLangOpts();
+  if (Opts.ObjC)
+    // FIXME: Revise how this check could operate on ObjC code.
+    return;
+
+  Finder->addMatcher(functionDecl(
+                         // Only report for definitions as the user only has
+                         // chance to fix if they can change the def.
+                         isDefinition(), hasAtLeastNumParams(MinimumLength),
+                         unless(ast_matchers::isTemplateInstantiation()))
+                         .bind("fun"),
+                     this);
+
+  Finder->addMatcher(functionDecl(
+                         // Only report for definitions as the user only has
+                         // chance to fix if they can change the def.
+                         isDefinition(), hasAtLeastNumParams(MinimumLength),
+                         isExplicitTemplateSpecialization())
+                         .bind("fun"),
+                     this);
+}
+
+/// Returns whether the given Mixup, when diagnosed, should elaborate the type
+/// of the arguments involved.
+static bool NeedsToPrintType(const Mixup &M) {
+  return M.Flags & (MIXUP_Typedef | MIXUP_RefBind | MIXUP_CVR);
+}
+
+void CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Fun = Result.Nodes.getNodeAs<FunctionDecl>("fun");
+
+  unsigned int ParamMixRangeStartIdx = 0;
+  const unsigned int NumArgs = Fun->getNumParams();
+
+  while (ParamMixRangeStartIdx < NumArgs) {
+    if (isIgnored(Fun->getParamDecl(ParamMixRangeStartIdx))) {
+      // If the current parameters's name or type name is ignored, don't try
+      // creating a range from it.
+      ++ParamMixRangeStartIdx;
+      continue;
+    }
+
+    MixableAdjacentParamRange MixingRange =
+        BuildMixableParameterRange(*this, Fun, ParamMixRangeStartIdx);
+    assert(MixingRange.NumParamsChecked > 0 && "Ensure continuity!");
+    ParamMixRangeStartIdx += MixingRange.NumParamsChecked;
+    if (MixingRange.NumParamsChecked < MinimumLength)
+      continue;
+
+    // A function with an adjacent argument range of sufficient length found.
+    std::string FunName = Fun->getDeclName().getAsString();
+    if (FunName.empty())
+      FunName = "<unknown>";
+
+    const PrintingPolicy &PP{Fun->getASTContext().getLangOpts()};
+    std::string MainParmTypeAsWritten =
+        MixingRange.getFirstParm()->getType().getAsString(PP);
+
+    bool HasAnyTypePrint = llvm::any_of(MixingRange.Mixups, NeedsToPrintType);
+
+    {
+      const ParmVarDecl *RangeFirst = MixingRange.getFirstParm();
+      const ParmVarDecl *RangeLast = MixingRange.getLastParm();
+
+      {
+        StringRef MainDiagnostic;
+        if (HasAnyTypePrint)
+          MainDiagnostic = "%0 adjacent parameters for '%1' of similar type "
+                           "are easily swapped "
+                           "by mistake";
+        else
+          MainDiagnostic = "%0 adjacent parameters for '%1' of similar type "
+                           "('%2') are easily "
+                           "swapped by mistake";
+
+        auto Diag = diag(RangeFirst->getOuterLocStart(), MainDiagnostic)
+                    << static_cast<unsigned>(MixingRange.NumParamsChecked)
+                    << FunName;
+        if (!HasAnyTypePrint)
+          Diag << MainParmTypeAsWritten;
+      }
+
+      // Unfortunately, undersquiggly highlights of ranges is not supported by
+      // ClangTidyDiagnosticConsumer without a fake fixit...
+      diag(RangeFirst->getLocation(),
+           "the first parameter in this range is '%0'", DiagnosticIDs::Note)
+          << (!RangeFirst->getName().empty() ? RangeFirst->getName()
+                                             : "<unnamed>");
+      diag(RangeLast->getLocation(), "the last parameter in this range is '%0'",
+           DiagnosticIDs::Note)
+          << (!RangeLast->getName().empty() ? RangeLast->getName()
+                                            : "<unnamed>");
+    }
+
+    llvm::SmallPtrSet<const ParmVarDecl *, 4> TypedefResolutionPrintedForParm;
+    llvm::SmallPtrSet<const ParmVarDecl *, 4> CVRNotePrintedForParm;
+    for (const Mixup &M : MixingRange.Mixups) {
+      assert(M.Flags >= MIXUP_Trivial && "Too low bits in mixup type.");
+      // For MIXUP_Trivial no extra diagnostics required.
+
+      std::string FirstParmType, SecondParmType;
+      if (NeedsToPrintType(M)) {
+        // Typedefs, and reference binds might result in the type of a variable
+        // printed in the diagnostic, so we have to prepare it.
+        FirstParmType = TypeNameAsString(M.First->getType(), PP);
+        SecondParmType = TypeNameAsString(M.Second->getType(), PP);
+      }
+
+      if (NeedsToPrintType(M)) {
+        if (M.Flags & MIXUP_Typedef) {
+          // FIXME: Don't emit the typedef note for the parameter that isn't
+          // actually a typedef.
+          if (!TypedefResolutionPrintedForParm.count(M.First)) {
+            diag(M.First->getOuterLocStart(),
+                 "after resolving type aliases, type of parameter '%0' is '%1'",
+                 DiagnosticIDs::Note)
+                << M.First->getName() << FirstParmType;
+            TypedefResolutionPrintedForParm.insert(M.First);
+          }
+
+          if (!TypedefResolutionPrintedForParm.count(M.Second)) {
+            diag(M.Second->getOuterLocStart(),
+                 "after resolving type aliases, type of parameter '%0' is '%1'",
+                 DiagnosticIDs::Note)
+                << M.Second->getName() << SecondParmType;
+            TypedefResolutionPrintedForParm.insert(M.Second);
+          }
+        }
+
+        if (M.Flags & (MIXUP_RefBind | MIXUP_CVR)) {
+          if (!CVRNotePrintedForParm.count(M.Second)) {
+            diag(M.Second->getOuterLocStart(),
+                 "at a call site, '%0' might bind with same force as '%1'",
+                 DiagnosticIDs::Note)
+                << SecondParmType << MainParmTypeAsWritten;
+            CVRNotePrintedForParm.insert(M.Second);
+          }
+        }
+      }
+    }
+  }
+}
+
+} // namespace experimental
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/experimental/ExperimentalTidyModule.cpp

 //===-- ExperimentalTidyModule.cpp - clang-tidy ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace experimental {
 
 /// A module containing checks that are experimental versions of future checks
 /// that belong to other modules.
 class ExperimentalModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
-
+    CheckFactories.registerCheck<
+        CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck>(
+        "experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-"
+        "type");
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ExperimentalModule>
     X("experimental-module",
       "Adds checks for that are experimental versions of future checks.");
 
 } // namespace experimental
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ExperimentalModule.
 volatile int ExperimentalModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 New module
 ^^^^^^^^^^
 - New module `llvmlibc`.
 
   This module contains checks related to the LLVM-libc coding standards.
 
 New checks
 ^^^^^^^^^^
+
 - New :doc:`cppcoreguidelines-avoid-non-const-global-variables
   <clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables>` check.
   Finds non-const global variables as described in check I.2 of C++ Core
   Guidelines.
 
 - New :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc
   <clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc>` check.
 
- New :doc:`bugprone-suspicious-include
   them.
 
 - New :doc:`cert-oop57-cpp
   <clang-tidy/checks/cert-oop57-cpp>` check.
 
   Flags use of the `C` standard library functions ``memset``, ``memcpy`` and
   ``memcmp`` and similar derivatives on non-trivial types.
 
+- New :doc:`experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type
+  <clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type>`
+  check.
+
+  Finds function definitions where parameters of the same type follow each other
+  directly, making call sites prone to calling the function with swapped or badly
+  ordered arguments.
+
 - New :doc:`llvmlibc-implementation-in-namespace
   <clang-tidy/checks/llvmlibc-implementation-in-namespace>` check.
 
   Checks all llvm-libc implementation is within the correct namespace.
 
 - New :doc:`llvmlibc-restrict-system-libc-headers
   <clang-tidy/checks/llvmlibc-restrict-system-libc-headers>` check.
 
   Finds includes of system libc headers not provided by the compiler within
   llvm-libc implementations.
 
 - New :doc:`objc-dealloc-in-category
   <clang-tidy/checks/objc-dealloc-in-category>` check.
 
   Finds implementations of -dealloc in Objective-C categories.
 
 - New :doc:`misc-no-recursion
   <clang-tidy/checks/misc-no-recursion>` check.
 
   Finds recursive functions and diagnoses them.
 
 - New :doc:`objc-nsinvocation-argument-lifetime
   <clang-tidy/checks/objc-nsinvocation-argument-lifetime>` check.
 
   Finds calls to ``NSInvocation`` methods under ARC that don't have proper
   argument object lifetimes.
 
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-con36-c

clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst

+.. title:: clang-tidy - experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type
+
+experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type
+=========================================================================
+
+Finds function definitions where parameters of the same type follow each other
+directly, making call sites prone to calling the function with swapped or badly
+ordered arguments.
+
+The rule is part of the "Interfaces" profile of the C++ Core Guidelines, see
+`I.24 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#i24-avoid-adjacent-unrelated-parameters-of-the-same-type>`_.
+
+.. code-block:: c++
+
+  void draw_point(int X, int Y) {}
+  FILE *open_path(const char *Dir, const char *Name, Flags Mode) {}
+
+A potential call like ``draw(-2, 5)`` or
+``open_path("a.txt", "/tmp", Read | Write)`` is perfectly legal from the
+language's perspective, but might not be what the developer of the function
+intended.
+
+The C++ Core Guidelines recommend using more elaborate types for parameters,
+such as
+
+.. code-block:: c++
+
+    struct Coords2D { int X; int Y; };
+    void draw_point(const Coords2D Point) {}
+
+    FILE *open_path(const Path &Dir, const Filename &Name, Flags Mode) {}
+
+Due to the elaborate refactoring and API-breaking requirements posed by fixing
+the issues diagnosed by this check, **no automatic fix-its** are proposed.
+
+Currently, functions that take pairs of iterators (where the suggested option
+would be moving to, e.g., the
+`Ranges <http://en.cppreference.com/w/cpp/ranges>`_ library) are hard-coded to
+not produce a diagnostic.
+
+ - The following *parameter names* and their Uppercase-initial variants are
+   ignored:
+   ``iterator``, ``begin``, ``end``, ``first``, ``last``.
+ - The following *type names* and their lowercase-initial variants (for types
+   of any parameters found) are ignored:
+   ``It``, ``Iterator``, ``InputIt``, ``ForwardIt``, ``BidirIt``,
+   ``const_iterator``, ``ConstIterator``
+
+Options
+-------
+
+.. option:: MinimumLength
+
+    The minimum length of the adjacent parameter sequence required before a
+    diagnostic is emitted.
+    Defaults to `2`.
+    Should be any integer number that's at least 2.
+    If `0` or `1` is given, the check will run as if `2` was specified.
+
+.. option:: IgnoredNames
+
+    The list of parameter names that should not be considered part of a mixable
+    parameter range by the checker.
+    Parameters bearing ignored names will be skipped.
+    The value should be a ``;``-separated list of names.
+    The option is case-sensitive!
+
+    By default, the following, and their Uppercase-initial variants are ignored:
+    ``iterator``, ``begin``, ``end``, ``first``, ``last``.
+
+.. option:: IgnoredTypes
+
+    The list of parameter type suffices that should not be considered part of a
+    mixable parameter range by the checker.
+    Parameters of type bearing an ignored type name (or which suffix is found in
+    the list) will be skipped.
+    The value should be a ``;``-separated list of names.
+    The option is case-sensitive!
+
+    By default, the following, and their lowercase-initial variants are ignored:
+    ``It``, ``Iterator``, ``InputIt``, ``ForwardIt``, ``BidirIt``,
+    ``const_iterator``, ``ConstIterator``.
+
+.. option:: CVRMixPossible
+
+    Whether to consider parameters of type ``T`` and the qualified
+    ``const T``/``volatile T`` counterpart forming a common "adjacent
+    parameter" sequence.
+    If `0` (default value), the check assumes such parameters cannot be mixed
+    up at a potential call site.
+    A non-zero value turns this assumption **off**.
+    A non-zero value is generally expected to produce a broader set of results.
+
+    The following example will not produce a diagnostic unless
+    *CVRMixPossible* is set to a non-zero value.
+
+    .. code-block:: c++
+
+        struct T {};
+        void f(T *tp, const T *ctp) {}
+
+Limitations
+-----------
+
+This check does not investigate functions that were instantiated from function
+templates.
+As such, only the primary template definitions and explicit specialisations are
+checked.
+
+Furthermore, this check is not able to find if some parameters of
+template-dependent types might end up referring to the same type in the end,
+creating an adjacent range that could be mixed up.
+
+The following example will not be warned about.
+
+.. code-block:: c++
+
+    template <typename T>
+    struct list_node {
+        T value;
+        list_node<T> *prev, *next;
+    };
+
+    template <typename T>
+    list_node<T> *create(const T& value,
+                         const list_node<T> *prev,
+                         const list_node<T> *next) { /* ... */ }
+
+The following example will not be warned about, despite it should be, as the
+last parameters to the function in the example are of type ``const T &``.
+
+.. code-block:: c++
+
+    template <typename T>
+    struct vector {
+        typedef       T          element_type;
+        typedef const T    const_element_type;
+        typedef       T &        reference_type;
+        typedef const T &  const_reference_type;
+    };
+
+    // Finds the longest occurrence's length between elements "RightEnd"
+    // and "LeftBegin". For example, in a vector of
+    //     <1, 2, 3, 4, 2, 8, 8, 8, 8, 4>
+    // findLongestOccurrenceLengthBetween(2, 4) should return 4 (the 4 8s).
+    template <typename T>
+    unsigned int findLongestOccurrenceLengthBetween(
+        const          vector<T>                       & Vector,
+              typename vector<T>::const_reference_type   RightEnd,
+        const typename vector<T>::element_type         & LeftBegin) { /* ... */ }

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `cppcoreguidelines-pro-type-reinterpret-cast <cppcoreguidelines-pro-type-reinterpret-cast.html>`_,
    `cppcoreguidelines-pro-type-static-cast-downcast <cppcoreguidelines-pro-type-static-cast-downcast.html>`_, "Yes"
    `cppcoreguidelines-pro-type-union-access <cppcoreguidelines-pro-type-union-access.html>`_,
    `cppcoreguidelines-pro-type-vararg <cppcoreguidelines-pro-type-vararg.html>`_,
    `cppcoreguidelines-slicing <cppcoreguidelines-slicing.html>`_,
    `cppcoreguidelines-special-member-functions <cppcoreguidelines-special-member-functions.html>`_,
    `darwin-avoid-spinlock <darwin-avoid-spinlock.html>`_,
    `darwin-dispatch-once-nonstatic <darwin-dispatch-once-nonstatic.html>`_, "Yes"
+   `experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type <experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.html>`_,
    `fuchsia-default-arguments-calls <fuchsia-default-arguments-calls.html>`_,
    `fuchsia-default-arguments-declarations <fuchsia-default-arguments-declarations.html>`_, "Yes"
    `fuchsia-multiple-inheritance <fuchsia-multiple-inheritance.html>`_,
    `fuchsia-overloaded-operator <fuchsia-overloaded-operator.html>`_,
    `fuchsia-statically-constructed-objects <fuchsia-statically-constructed-objects.html>`_,
    `fuchsia-trailing-return <fuchsia-trailing-return.html>`_,
    `fuchsia-virtual-inheritance <fuchsia-virtual-inheritance.html>`_,
    `google-build-explicit-make-pair <google-build-explicit-make-pair.html>`_,

clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp

+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \
+// RUN:   ]}' --
+
+void library(void *vp, void *vp2, void *vp3, int n, int m);
+// NO-WARN: The user has no chance to change only declared (usually library)
+// functions, so no diagnostic is made.
+
+struct T {};
+
+void create(T **out_t) {} // NO-WARN
+
+void unnamed_param(T *p, T *) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 2 adjacent parameters for 'unnamed_param' of similar type ('T *') are easily swapped by mistake [experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type]
+// CHECK-MESSAGES: :[[@LINE-2]]:23: note: the first parameter in this range is 'p'
+// CHECK-MESSAGES: :[[@LINE-3]]:29: note: the last parameter in this range is '<unnamed>'
+
+void copy(T *p, T *q, int n) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 2 adjacent parameters for 'copy' of similar type ('T *') are
+// CHECK-MESSAGES: :[[@LINE-2]]:14: note: the first parameter in this range is 'p'
+// CHECK-MESSAGES: :[[@LINE-3]]:20: note: the last parameter in this range is 'q'
+
+void mov(T *dst, const T *src) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 2 adjacent parameters for 'mov' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:13: note: the first parameter in this range is 'dst'
+// CHECK-MESSAGES: :[[@LINE-3]]:27: note: the last parameter in this range is 'src'
+// CHECK-MESSAGES: :[[@LINE-4]]:18: note: at a call site, 'const T *' might bind with same force as 'T *'
+
+void compare01(T t1, T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare01' of similar type ('T') are
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:24: note: the last parameter in this range is 't2'
+
+void compare02(const T t1, T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare02' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in this range is 't2'
+// CHECK-MESSAGES: :[[@LINE-4]]:28: note: at a call site, 'T' might bind with same force as 'const T'
+
+void compare03(T t1, const T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare03' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in this range is 't2'
+// CHECK-MESSAGES: :[[@LINE-4]]:22: note: at a call site, 'const T' might bind with same force as 'T'
+
+void compare04(const T &t1, T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare04' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:31: note: the last parameter in this range is 't2'
+// CHECK-MESSAGES: :[[@LINE-4]]:29: note: at a call site, 'T' might bind with same force as 'const T &'
+
+void compare05(T t1, const T &t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare05' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:31: note: the last parameter in this range is 't2'
+// CHECK-MESSAGES: :[[@LINE-4]]:22: note: at a call site, 'const T &' might bind with same force as 'T'
+
+void compare06(const T &t1, const T &t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare06' of similar type ('const T &') are
+// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in this range is 't2'
+
+void compare07(const T &t1, const T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare07' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:37: note: the last parameter in this range is 't2'
+// CHECK-MESSAGES: :[[@LINE-4]]:29: note: at a call site, 'const T' might bind with same force as 'const T &'
+
+void compare08(const T t1, const T &t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare08' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:37: note: the last parameter in this range is 't2'
+// CHECK-MESSAGES: :[[@LINE-4]]:28: note: at a call site, 'const T &' might bind with same force as 'const T'
+
+// In general, rvalue references and copies are hard to mess up at call sites.
+void compare09(T &&t1, T t2) {} // NO-WARN.
+void compare0A(T t1, T &&t2) {} // NO-WARN.
+
+void compare0B(T &&t1, T &&t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'compare0B' of similar type ('T &&') are
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:28: note: the last parameter in this range is 't2'
+
+struct U {};
+void compare0C(T t, U u) {}               // NO-WARN.
+void compare0D(const T t, U u) {}         // NO-WARN.
+void compare0E(T t, const U u) {}         // NO-WARN.
+void compare0F(const U &u, T t) {}        // NO-WARN.
+void compare10(T t, const U &u) {}        // NO-WARN.
+void compare11(const T &t, const U &u) {} // NO-WARN.
+void compare12(const T &t, const U u) {}  // NO-WARN.
+void compare13(const U u, const T &t) {}  // NO-WARN.
+void compare14(T &&t, U u) {}             // NO-WARN.
+void compare15(T t, U &&u) {}             // NO-WARN.
+
+void multi_bind(T t, const T t2, const T &t3, T &&t4) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 3 adjacent parameters for 'multi_bind' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:19: note: the first parameter in this range is 't'
+// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in this range is 't3'
+// CHECK-MESSAGES: :[[@LINE-4]]:22: note: at a call site, 'const T' might bind with same force as 'T'
+// CHECK-MESSAGES: :[[@LINE-5]]:34: note: at a call site, 'const T &' might bind with same force as 'T'
+
+void ptr_quals(int *ip, const int *cip, volatile int *vip, int *const ipc, volatile int *volatile vipv) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 5 adjacent parameters for 'ptr_quals' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'ip'
+// CHECK-MESSAGES: :[[@LINE-3]]:99: note: the last parameter in this range is 'vipv'
+// CHECK-MESSAGES: :[[@LINE-4]]:25: note: at a call site, 'const int *' might bind with same force as 'int *'
+// CHECK-MESSAGES: :[[@LINE-5]]:41: note: at a call site, 'volatile int *' might bind with same force as 'int *'
+// CHECK-MESSAGES: :[[@LINE-6]]:60: note: at a call site, 'int * const' might bind with same force as 'int *'
+// CHECK-MESSAGES: :[[@LINE-7]]:76: note: at a call site, 'volatile int * volatile' might bind with same force as 'int *'
+
+void value_quals(T t, const T ct, volatile T vt, const volatile T cvt) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 4 adjacent parameters for 'value_quals' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in this range is 't'
+// CHECK-MESSAGES: :[[@LINE-3]]:67: note: the last parameter in this range is 'cvt'
+// CHECK-MESSAGES: :[[@LINE-4]]:23: note: at a call site, 'const T' might bind with same force as 'T'
+// CHECK-MESSAGES: :[[@LINE-5]]:35: note: at a call site, 'volatile T' might bind with same force as 'T'
+// CHECK-MESSAGES: :[[@LINE-6]]:50: note: at a call site, 'const volatile T' might bind with same force as 'T'
+
+void value_ptr_quals(T *tp, const T *ctp, volatile T *vtp, const volatile T *cvtp, T *const tpc, T *volatile tpv) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 6 adjacent parameters for 'value_ptr_quals' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in this range is 'tp'
+// CHECK-MESSAGES: :[[@LINE-3]]:110: note: the last parameter in this range is 'tpv'
+// CHECK-MESSAGES: :[[@LINE-4]]:29: note: at a call site, 'const T *' might bind with same force as 'T *'
+// CHECK-MESSAGES: :[[@LINE-5]]:43: note: at a call site, 'volatile T *' might bind with same force as 'T *'
+// CHECK-MESSAGES: :[[@LINE-6]]:60: note: at a call site, 'const volatile T *' might bind with same force as 'T *'
+// CHECK-MESSAGES: :[[@LINE-7]]:84: note: at a call site, 'T * const' might bind with same force as 'T *'
+// CHECK-MESSAGES: :[[@LINE-8]]:98: note: at a call site, 'T * volatile' might bind with same force as 'T *'
+
+typedef int Numeral;
+void set_lerped(const Numeral *const min,
+                const Numeral *const max,
+                Numeral *const value) {}
+// CHECK-MESSAGES: :[[@LINE-3]]:17: warning: 3 adjacent parameters for 'set_lerped' of similar type are
+// CHECK-MESSAGES: :[[@LINE-4]]:38: note: the first parameter in this range is 'min'
+// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in this range is 'value'
+// CHECK-MESSAGES: :[[@LINE-4]]:17: note: at a call site, 'int * const' might bind with same force as 'const Numeral *const'
+
+typedef int I1;
+typedef int I2;
+void multi_through_typedef(I1 i1, I2 i2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 2 adjacent parameters for 'multi_through_typedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:31: note: the first parameter in this range is 'i1'
+// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in this range is 'i2'
+// CHECK-MESSAGES: :[[@LINE-4]]:28: note: after resolving type aliases, type of parameter 'i1' is 'int'
+// CHECK-MESSAGES: :[[@LINE-5]]:35: note: after resolving type aliases, type of parameter 'i2' is 'int'
+
+void multi_ptr_through_typedef(I1 *p1, I2 *p2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: 2 adjacent parameters for 'multi_ptr_through_typedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in this range is 'p1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in this range is 'p2'
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: after resolving type aliases, type of parameter 'p1' is 'int *'
+// CHECK-MESSAGES: :[[@LINE-5]]:40: note: after resolving type aliases, type of parameter 'p2' is 'int *'
+
+void typedef_and_realtype_ptr_const(int *p1, const I1 *p2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 2 adjacent parameters for 'typedef_and_realtype_ptr_const' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:42: note: the first parameter in this range is 'p1'
+// CHECK-MESSAGES: :[[@LINE-3]]:56: note: the last parameter in this range is 'p2'
+// CHECK-MESSAGES: :[[@LINE-4]]:37: note: after resolving type aliases, type of parameter 'p1' is 'int *'
+// CHECK-MESSAGES: :[[@LINE-5]]:46: note: after resolving type aliases, type of parameter 'p2' is 'const int *'
+// CHECK-MESSAGES: :[[@LINE-6]]:46: note: at a call site, 'const int *' might bind with same force as 'int *'
+
+void typedef_ptr_const(I1 *p1, const I1 *p2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters for 'typedef_ptr_const' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in this range is 'p1'
+// CHECK-MESSAGES: :[[@LINE-3]]:42: note: the last parameter in this range is 'p2'
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: at a call site, 'const int *' might bind with same force as 'I1 *'
+
+typedef I2 myInt;
+typedef I2 myOtherInt;
+void typedef_chain(myInt mi, myOtherInt moi) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 2 adjacent parameters for 'typedef_chain' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:26: note: the first parameter in this range is 'mi'
+// CHECK-MESSAGES: :[[@LINE-3]]:41: note: the last parameter in this range is 'moi'
+// CHECK-MESSAGES: :[[@LINE-4]]:20: note: after resolving type aliases, type of parameter 'mi' is 'int'
+// CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, type of parameter 'moi' is 'int'
+
+void bind_power_and_typedef(int Read, const myOtherInt &Write) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent parameters for 'bind_power_and_typedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:33: note: the first parameter in this range is 'Read'
+// CHECK-MESSAGES: :[[@LINE-3]]:57: note: the last parameter in this range is 'Write'
+// CHECK-MESSAGES: :[[@LINE-4]]:39: note: after resolving type aliases, type of parameter 'Write' is 'const int &'
+// CHECK-MESSAGES: :[[@LINE-5]]:39: note: at a call site, 'const int &' might bind with same force as 'int'
+
+typedef myOtherInt *IP;
+typedef const IP &cipr;
+void bind_power_and_multi_layer_typedef(int *RP, cipr WP) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:41: warning: 2 adjacent parameters for 'bind_power_and_multi_layer_typedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:46: note: the first parameter in this range is 'RP'
+// CHECK-MESSAGES: :[[@LINE-3]]:55: note: the last parameter in this range is 'WP'
+// CHECK-MESSAGES: :[[@LINE-4]]:50: note: after resolving type aliases, type of parameter 'WP' is 'const int * &'
+// CHECK-MESSAGES: :[[@LINE-5]]:50: note: at a call site, 'const int * &' might bind with same force as 'int *'
+
+void entirely_different(int i, long l) {} // NO-WARN.
+
+struct StringRef {
+  char *Data;
+};
+typedef bool Toggle;
+
+void multi_pack(StringRef Path, StringRef Name, int Size, Toggle Read, Toggle Write) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters for 'multi_pack' of similar type ('StringRef') are
+// CHECK-MESSAGES: :[[@LINE-2]]:27: note: the first parameter in this range is 'Path'
+// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in this range is 'Name'
+// CHECK-MESSAGES: :[[@LINE-4]]:59: warning: 2 adjacent parameters for 'multi_pack' of similar type ('Toggle') are
+// CHECK-MESSAGES: :[[@LINE-5]]:66: note: the first parameter in this range is 'Read'
+// CHECK-MESSAGES: :[[@LINE-6]]:79: note: the last parameter in this range is 'Write'
+
+void protochain(int, int, int);
+void protochain(int i, int, int);
+void protochain(int, int j, int);
+void protochain(int i, int j, int k) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 3 adjacent parameters for 'protochain' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'i'
+// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in this range is 'k'
+
+void protochain2(I1, I2, myInt);
+void protochain2(I1 i, I2, myInt);
+void protochain2(I1, I2 j, int);
+void protochain2(I1 i, I2 j, myOtherInt k) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 3 adjacent parameters for 'protochain2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'i'
+// CHECK-MESSAGES: :[[@LINE-3]]:41: note: the last parameter in this range is 'k'
+// CHECK-MESSAGES: :[[@LINE-4]]:18: note: after resolving type aliases, type of parameter 'i' is 'int'
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: after resolving type aliases, type of parameter 'j' is 'int'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: after resolving type aliases, type of parameter 'k' is 'int'
+
+void defined(int a, int b);
+void defined(int i, int j) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters for 'defined' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in this range is 'i'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in this range is 'j'
+
+template <typename T>
+void ptr_pair(T *p, T *q) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters for 'ptr_pair' of similar type ('T *') are
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in this range is 'p'
+// CHECK-MESSAGES: :[[@LINE-3]]:24: note: the last parameter in this range is 'q'
+
+template <typename T>
+void cptr_pair(const T *p, const T *q) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent parameters for 'cptr_pair' of similar type ('const T *') are
+// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in this range is 'p'
+// CHECK-MESSAGES: :[[@LINE-3]]:37: note: the last parameter in this range is 'q'
+
+void ref_and_typedef(int i, I2 i2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 2 adjacent parameters for 'ref_and_typedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:26: note: the first parameter in this range is 'i'
+// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in this range is 'i2'
+// CHECK-MESSAGES: :[[@LINE-4]]:22: note: after resolving type aliases, type of parameter 'i' is 'int'
+// CHECK-MESSAGES: :[[@LINE-5]]:29: note: after resolving type aliases, type of parameter 'i2' is 'int'
+
+void ref_and_typedef2(I1 i, int i2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 2 adjacent parameters for 'ref_and_typedef2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:26: note: the first parameter in this range is 'i'
+// CHECK-MESSAGES: :[[@LINE-3]]:33: note: the last parameter in this range is 'i2'
+// CHECK-MESSAGES: :[[@LINE-4]]:23: note: after resolving type aliases, type of parameter 'i' is 'int'
+// CHECK-MESSAGES: :[[@LINE-5]]:29: note: after resolving type aliases, type of parameter 'i2' is 'int'
+
+int strcmp(const char *A, const char *B) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent parameters for 'strcmp' of similar type ('const char *') are
+// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in this range is 'A'
+// CHECK-MESSAGES: :[[@LINE-3]]:39: note: the last parameter in this range is 'B'
+
+using str = char *;
+using strliteral = const char *;
+int str_compare(strliteral a, strliteral b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters for 'str_compare' of similar type ('strliteral') are
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in this range is 'a'
+// CHECK-MESSAGES: :[[@LINE-3]]:42: note: the last parameter in this range is 'b'
+
+void append(str s, char *s2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'append' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:17: note: the first parameter in this range is 's'
+// CHECK-MESSAGES: :[[@LINE-3]]:26: note: the last parameter in this range is 's2'
+// CHECK-MESSAGES: :[[@LINE-4]]:13: note: after resolving type aliases, type of parameter 's' is 'char *'
+// CHECK-MESSAGES: :[[@LINE-5]]:20: note: after resolving type aliases, type of parameter 's2' is 'char *'
+
+void append_lit(str s, strliteral s2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters for 'append_lit' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 's'
+// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in this range is 's2'
+// CHECK-MESSAGES: :[[@LINE-4]]:17: note: after resolving type aliases, type of parameter 's' is 'char *'
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: after resolving type aliases, type of parameter 's2' is 'const char *'
+
+void swap(str *a, char **b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 2 adjacent parameters for 'swap' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:16: note: the first parameter in this range is 'a'
+// CHECK-MESSAGES: :[[@LINE-3]]:26: note: the last parameter in this range is 'b'
+// CHECK-MESSAGES: :[[@LINE-4]]:11: note: after resolving type aliases, type of parameter 'a' is 'char * *'
+// CHECK-MESSAGES: :[[@LINE-5]]:19: note: after resolving type aliases, type of parameter 'b' is 'char * *'
+
+template <typename I> // 'I' as template name not on ignore list.
+void shortthing(I a, I b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters for 'shortthing' of similar type ('I') are
+// CHECK-MESSAGES: :[[@LINE-2]]:19: note: the first parameter in this range is 'a'
+// CHECK-MESSAGES: :[[@LINE-3]]:24: note: the last parameter in this range is 'b'
+
+// FIXME: Suggest Ranges library alternative?
+// NO-WARN: Ignore common type packs, like iterators.
+template <typename InputIt>
+void something(InputIt a, InputIt b) {}
+
+// NO-WARN: Ignore common names, like for iterators.
+template <typename InputIt, typename E>
+void find(InputIt first, InputIt last, const E &Element) {}
+
+template <typename T>
+struct vector {
+  T *Data;
+  unsigned N;
+  T operator[](unsigned Idx) { return *(Data + N); }
+
+  typedef T *iterator;
+  typedef T element;
+  typedef const T &element_cref;
+};
+
+// NO-WARN: parameter has "iterator" as type.
+template <typename T>
+typename vector<T>::iterator find2(
+    typename vector<T>::iterator from,
+    typename vector<T>::iterator to,
+    const T &E) {}
+
+template <typename K, typename V>
+struct map {
+  K *Keys;
+  V *Values;
+
+  struct map_iterator {
+    K *first;
+    V *second;
+  };
+  typedef map_iterator iterator;
+};
+
+// NO-WARN: parameter has "iterator" as type.
+template <typename K, typename V>
+typename map<K, V>::iterator find_m(
+    typename map<K, V>::iterator left,  // Purposefully named as such so param
+    typename map<K, V>::iterator right, // name doesn't match, only type name.
+    const K &k, const V &v) {}
+
+// NO-WARN: parameters are dependent types, it could be that for a certain
+// 'T' they refer to (essentially) the same thing, and for other T, they don't.
+// (Think of std::vector<bool>, perhaps?)
+// It would be very expensive and tangy logic to deduce that in this small
+// test example, one parameter is 'T' and the 'const T &'.
+template <typename T>
+vector<typename vector<T>::iterator> findOccurrenceCases(
+    typename vector<T>::element E, typename vector<T>::element_cref ER) {}
+
+// FIXME: This case should be diagnosed (are `DependentNameTypes` equatable?)
+template <typename T>
+vector<vector<T>> equalRangesBetween(
+    const typename vector<T>::element &F,
+    typename vector<T>::element L) {}
+
+// NO-WARN: Totally different types.
+struct Decl;
+struct Expr;
+struct Stmt;
+void HandleASTNodes(Decl *D, Expr *E, Stmt *S) {}
+
+// NO-WARN: Different template specialisations of the same template.
+void concatenate_many_strings(StringRef Out,
+                              vector<StringRef> InRef,
+                              vector<str> InStr,
+                              vector<strliteral> InLiterals) {}
+
+// NO-WARN: Different template parameters.
+template <typename X, typename Y, typename Comp>
+int compare(const X &x, const Y &y, Comp C) { return C(x, y); }
+
+// WARN: Explicit specialisation's definition with similar parameters.
+template <typename Comp>
+int compare(int x, int y, Comp C) { return C(x, y); }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'compare' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-2]]:17: note: the first parameter in this range is 'x'
+// CHECK-MESSAGES: :[[@LINE-3]]:24: note: the last parameter in this range is 'y'
+
+void compare_test() {
+  // NO-WARN: Function call, not defintion.
+  compare("A", "B", &strcmp);
+}
+
+int definition_inside_another(vector<int> V) {
+  struct Sum {
+    Sum(int _i, int _j) : i(_i), j(_j) {}
+    // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: 2 adjacent parameters for 'Sum' of similar type ('int') are
+    // CHECK-MESSAGES: :[[@LINE-2]]:13: note: the first parameter in this range is '_i'
+    // CHECK-MESSAGES: :[[@LINE-3]]:21: note: the last parameter in this range is '_j'
+
+    int operator()() const { return i + j; }
+
+    int i, j;
+  };
+  Sum S{V[0], V[1]};
+
+  return S();
+}
+
+void len3(const StringRef Drive, const StringRef Dirs, const StringRef FileName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 adjacent parameters for 'len3' of similar type ('const StringRef') are
+// CHECK-MESSAGES: :[[@LINE-2]]:27: note: the first parameter in this range is 'Drive'
+// CHECK-MESSAGES: :[[@LINE-3]]:72: note: the last parameter in this range is 'FileName'
+
+void len4(const StringRef Drive, const StringRef Dirs, const StringRef FileName, const StringRef Extension) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 4 adjacent parameters for 'len4' of similar type ('const StringRef') are
+// CHECK-MESSAGES: :[[@LINE-2]]:27: note: the first parameter in this range is 'Drive'
+// CHECK-MESSAGES: :[[@LINE-3]]:98: note: the last parameter in this range is 'Extension'
+
+using MainT = int(int argc, strliteral argv, strliteral envp);
+int call_two_programs(MainT one, MainT two) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 2 adjacent parameters for 'call_two_programs' of similar type ('MainT *') are
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in this range is 'one'
+// CHECK-MESSAGES: :[[@LINE-3]]:40: note: the last parameter in this range is 'two'
+
+using MainT2 = int(int argc, strliteral argv, strliteral envp);
+int call_two_programs2(MainT p1, MainT2 p2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters for 'call_two_programs2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:30: note: the first parameter in this range is 'p1'
+// CHECK-MESSAGES: :[[@LINE-3]]:41: note: the last parameter in this range is 'p2'
+// CHECK-MESSAGES: :[[@LINE-4]]:24: note: after resolving type aliases, type of parameter 'p1' is 'int (int, const char *, const char *)'
+// CHECK-MESSAGES: :[[@LINE-5]]:34: note: after resolving type aliases, type of parameter 'p2' is 'int (int, const char *, const char *)'
+
+typedef struct {
+  int i;
+} Integer;
+bool int_equals(Integer i1, Integer i2) { return i1.i == i2.i; }
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters for 'int_equals' of similar type ('Integer') are
+// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in this range is 'i1'
+// CHECK-MESSAGES: :[[@LINE-3]]:37: note: the last parameter in this range is 'i2'
+
+typedef enum {
+  Exclamation,
+  Question,
+  Dot
+} Token;
+bool tok_equals(Token t1, Token t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters for 'tok_equals' of similar type ('Token') are
+// CHECK-MESSAGES: :[[@LINE-2]]:23: note: the first parameter in this range is 't1'
+// CHECK-MESSAGES: :[[@LINE-3]]:33: note: the last parameter in this range is 't2'
+
+enum E : char {
+  A = 'A',
+  B = 'B',
+  C = 'C'
+};
+
+// QUESTION: Should this warn?
+// Conversion checks in general may not belong to this check in particular.
+bool isSameLetter(E e, const char &c) {}
+
+template <typename T>
+struct doubly_linked_list_node {
+  T val;
+  doubly_linked_list_node<T> *prev;
+  doubly_linked_list_node<T> *next;
+};
+
+// FIXME: doubly_linked_list_node<T> is a dependent type, and thus doesn't warn.
+template <typename T>
+doubly_linked_list_node<T> *list_insert(const T &element,
+                                        const doubly_linked_list_node<T> *prev,
+                                        const doubly_linked_list_node<T> *next) {}
+
+template <class... Args>
+void fwd(Args &&... args, Args &&... args2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 2 adjacent parameters for 'fwd' of similar type ('Args &&...') are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'args'
+// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in this range is 'args2'
+
+// Make sure the logic for the check doesn't crash...
+
+// Matched case from '/usr/include/c++/7/bits/gthr-default.h'.
+typedef unsigned long int gthread_t;
+static inline int gthread_create(gthread_t *threadid,
+                                 void *(*func)(void *),
+                                 void *args) {}
+
+// A modification of the above function that should warn.
+int gthread_create_random(gthread_t *threadid,
+                          void *(*func_1)(void *),
+                          void *(*func_2)(void *),
+                          void *args) {}
+// CHECK-MESSAGES: :[[@LINE-3]]:27: warning: 2 adjacent parameters for 'gthread_create_random' of similar type ('void *(*)(void *)') are
+// CHECK-MESSAGES: :[[@LINE-4]]:35: note: the first parameter in this range is 'func_1'
+// CHECK-MESSAGES: :[[@LINE-4]]:35: note: the last parameter in this range is 'func_2'

clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-ignores.cpp

+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.IgnoredNames, value: "a;b"}, \
+// RUN:     {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.IgnoredTypes, value: "foo;bar"} \
+// RUN:   ]}' --
+
+struct iterator {};
+
+// These names are not ignored.
+void copy(iterator begin, iterator end) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 2 adjacent parameters for 'copy' of similar type ('iterator') are easily swapped by mistake [experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type]
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in this range is 'begin'
+// CHECK-MESSAGES: :[[@LINE-3]]:36: note: the last parameter in this range is 'end'
+
+struct foo {};
+struct bar {};
+
+// NO-WARN: Ignored parameter types.
+void test01(foo x, foo y) {}
+void test02(bar x, bar y) {}
+
+struct crowbar {};
+void test03(crowbar x, crowbar y) {} // NO-WARN: Ignored parameter type suffix.
+
+// NO-WARN: Ignored parameter names.
+void test04(int a, int b) {}
+
+void len05(int x, int y, int z, int u, int v) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 5 adjacent parameters for 'len05' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-2]]:16: note: the first parameter in this range is 'x'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in this range is 'v'
+
+void ignored_inside_range(int x, int y, int a, int u, int v) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 2 adjacent parameters for 'ignored_inside_range' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-2]]:31: note: the first parameter in this range is 'x'
+// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in this range is 'y'
+// CHECK-MESSAGES: :[[@LINE-4]]:48: warning: 2 adjacent parameters for 'ignored_inside_range' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-5]]:52: note: the first parameter in this range is 'u'
+// CHECK-MESSAGES: :[[@LINE-6]]:59: note: the last parameter in this range is 'v'

clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp

+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.MinimumLength, value: 3} \
+// RUN:   ]}' --
+
+void library(void *vp, void *vp2, void *vp3, int n, int m);
+// NO-WARN: The user has no chance to change only declared (usually library)
+// functions, so no diagnostic is made.
+
+// NO-WARN for any of these: only 2 parameters present.
+
+struct T {};
+
+void create(T **out_t) {}
+void copy(T *p, T *q, int n) {}
+void mov(T *dst, const T *src) {}
+
+void compare01(T t1, T t2) {}
+void compare02(const T t1, T t2) {}
+void compare03(T t1, const T t2) {}
+void compare04(const T &t1, T t2) {}
+void compare05(T t1, const T &t2) {}
+void compare06(const T &t1, const T &t2) {}
+void compare07(const T &t1, const T t2) {}
+void compare08(const T t1, const T &t2) {}
+
+void compare09(T &&t1, T t2) {}
+void compare0A(T t1, T &&t2) {}
+void compare0B(T &&t1, T &&t2) {}
+
+struct U {};
+void compare0C(T t, U u) {}
+void compare0D(const T t, U u) {}
+void compare0E(T t, const U u) {}
+void compare0F(const U &u, T t) {}
+void compare10(T t, const U &u) {}
+void compare11(const T &t, const U &u) {}
+void compare12(const T &t, const U u) {}
+void compare13(const U u, const T &t) {}
+void compare14(T &&t, U u) {}
+void compare15(T t, U &&u) {}
+
+typedef int I1;
+typedef int I2;
+void multi_through_typedef(I1 i1, I2 i2) {}
+
+void multi_ptr_through_typedef(I1 *p1, I2 *p2) {}
+
+typedef I2 myInt;
+typedef I2 myOtherInt;
+void typedef_chain(myInt mi, myOtherInt moi) {}
+
+void bind_power_and_typedef(int Read, const myOtherInt &Write) {}
+
+typedef myOtherInt *IP;
+typedef const IP &cipr;
+void bind_power_and_multi_layer_typedef(int *RP, cipr WP) {}
+
+void entirely_different(int i, long l) {}
+
+struct StringRef {
+  char *Data;
+};
+typedef bool Toggle;
+
+void multi_pack(StringRef Path, StringRef Name, int Size, Toggle Read, Toggle Write) {}
+
+void defined(int a, int b);
+void defined(int i, int j) {}
+
+template <typename T>
+void ptr_pair(T *p, T *q) {}
+
+template <typename T>
+void cptr_pair(const T *p, const T *q) {}
+
+void ref_and_typedef(int i, I2 i2) {}
+void ref_and_typedef2(I1 i, int i2) {}
+
+int strcmp(const char *A, const char *B) {}
+
+using str = char *;
+using strlit = const str;
+int str_compare(strlit a, strlit b) {}
+
+void append(str s, char *s2) {}
+
+void append_lit(str s, strlit s2) {}
+
+void swap(str *a, char **b) {}
+
+// END of NO-WARN.
+
+// NO-WARN: CVR-qualification-mixup is turned off in this test file.
+
+void merge(T *dst, const T *src1, const T *src2) {}
+
+void multi_bind(T t, const T t2, const T &t3, T &&t4) {}
+
+void ptr_quals(int *ip, const int *cip, volatile int *vip, int *const ipc, volatile int *volatile vipv) {}
+
+void value_quals(T t, const T ct, volatile T vt, const volatile T cvt) {}
+
+void value_ptr_quals(T *tp, const T *ctp, volatile T *vtp, const volatile T *cvtp, T *const tpc, T *volatile tpv) {}
+
+void typedef_and_realtype_ptr_const(int *p1, I1 *p2, const I1 *p3) {}
+
+void typedef_ptr_const(I1 *p1, I1 *p2, const I1 *p3) {}
+
+// END of NO-WARN.
+
+// NO-WARN: const T* and T* should not mix, even if coming through a typedef.
+// (min and max could be mixed up, but length requirement in this test is 3.)
+typedef int Numeral;
+void set_lerped(const Numeral *const min,
+                const Numeral *const max,
+                Numeral *const value) {}
+
+void protochain(int, int, int);
+void protochain(int i, int, int);
+void protochain(int, int j, int);
+void protochain(int i, int j, int k) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 3 adjacent parameters for 'protochain' of similar type ('int') are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'i'
+// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in this range is 'k'
+
+void protochain2(I1, I2, myInt);
+void protochain2(I1 i, I2, myInt);
+void protochain2(I1, I2 j, int);
+void protochain2(I1 i, I2 j, myOtherInt k) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 3 adjacent parameters for 'protochain2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'i'
+// CHECK-MESSAGES: :[[@LINE-3]]:41: note: the last parameter in this range is 'k'
+// CHECK-MESSAGES: :[[@LINE-4]]:18: note: after resolving type aliases, type of parameter 'i' is 'int'
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: after resolving type aliases, type of parameter 'j' is 'int'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: after resolving type aliases, type of parameter 'k' is 'int'
+
+void len3(const StringRef Drive, const StringRef Dirs, const StringRef FileName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 adjacent parameters for 'len3' of similar type ('const StringRef') are
+// CHECK-MESSAGES: :[[@LINE-2]]:27: note: the first parameter in this range is 'Drive'
+// CHECK-MESSAGES: :[[@LINE-3]]:72: note: the last parameter in this range is 'FileName'
+
+void len4(const StringRef Drive, const StringRef Dirs, const StringRef FileName, const StringRef Extension) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 4 adjacent parameters for 'len4' of similar type ('const StringRef') are
+// CHECK-MESSAGES: :[[@LINE-2]]:27: note: the first parameter in this range is 'Drive'
+// CHECK-MESSAGES: :[[@LINE-3]]:98: note: the last parameter in this range is 'Extension'
+
+void multi_packs(StringRef Path, StringRef Name, StringRef Occupation,
+                 int Size, int Width,
+                 Toggle Read, Toggle Write, Toggle Execute) {}
+// CHECK-MESSAGES: :[[@LINE-3]]:18: warning: 3 adjacent parameters for 'multi_packs' of similar type ('StringRef') are
+// CHECK-MESSAGES: :[[@LINE-4]]:28: note: the first parameter in this range is 'Path'
+// CHECK-MESSAGES: :[[@LINE-5]]:60: note: the last parameter in this range is 'Occupation'
+// CHECK-MESSAGES: :[[@LINE-4]]:18: warning: 3 adjacent parameters for 'multi_packs' of similar type ('Toggle') are
+// CHECK-MESSAGES: :[[@LINE-5]]:25: note: the first parameter in this range is 'Read'
+// CHECK-MESSAGES: :[[@LINE-6]]:52: note: the last parameter in this range is 'Execute'
+
+// CVRMixupPossible only concerns 'T' and 'c/v/r T', or pointers thereof, not
+// references.
+void lifetime_extension(T t, const T &tr1, const T &tr2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3 adjacent parameters for 'lifetime_extension' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:27: note: the first parameter in this range is 't'
+// CHECK-MESSAGES: :[[@LINE-3]]:53: note: the last parameter in this range is 'tr2'
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: at a call site, 'const T &' might bind with same force as 'T'
+// CHECK-MESSAGES: :[[@LINE-5]]:44: note: at a call site, 'const T &' might bind with same force as 'T'
+
+enum OldSchoolTermColour {
+  Black,
+  Blue,
+  Green,
+  Cyan,
+  Red,
+  Purple,
+  BrownOrange,
+  LightGreyWhite,
+  Gray,
+  LightBlue,
+  LightGreen,
+  LightCyan,
+  LightRed,
+  LightPurple,
+  YellowLightOrange,
+  WhiteLightWhite
+};
+
+void setColouredOutput(OldSchoolTermColour Background,
+                       OldSchoolTermColour Foreground,
+                       OldSchoolTermColour Border) {}
+// CHECK-MESSAGES: :[[@LINE-3]]:24: warning: 3 adjacent parameters for 'setColouredOutput' of similar type ('OldSchoolTermColour') are
+// CHECK-MESSAGES: :[[@LINE-4]]:44: note: the first parameter in this range is 'Background'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in this range is 'Border'
+
+typedef OldSchoolTermColour BackColour;
+typedef OldSchoolTermColour ForeColour;
+typedef OldSchoolTermColour BorderColour;
+void setColouredOutput2(BackColour Background,
+                        ForeColour Foreground,
+                        BorderColour Border) {}
+// CHECK-MESSAGES: :[[@LINE-3]]:25: warning: 3 adjacent parameters for 'setColouredOutput2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-4]]:36: note: the first parameter in this range is 'Background'
+// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in this range is 'Border'
+// CHECK-MESSAGES: :[[@LINE-6]]:25: note: after resolving type aliases, type of parameter 'Background' is 'OldSchoolTermColour'
+// CHECK-MESSAGES: :[[@LINE-6]]:25: note: after resolving type aliases, type of parameter 'Foreground' is 'OldSchoolTermColour'
+// CHECK-MESSAGES: :[[@LINE-6]]:25: note: after resolving type aliases, type of parameter 'Border' is 'OldSchoolTermColour'

clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c

+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \
+// RUN:   ]}' --
+
+struct T {};
+
+void memcpy(struct T *restrict dest, const struct T *restrict src) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'memcpy' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in this range is 'dest'
+// CHECK-MESSAGES: :[[@LINE-3]]:63: note: the last parameter in this range is 'src'
+// CHECK-MESSAGES: :[[@LINE-4]]:38: note: at a call site, 'const struct T * restrict' might bind with same force as 'struct T *restrict'
+
+void merge(struct T *dst, const struct T *src1, const struct T *src2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 3 adjacent parameters for 'merge' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in this range is 'dst'
+// CHECK-MESSAGES: :[[@LINE-3]]:65: note: the last parameter in this range is 'src2'
+// CHECK-MESSAGES: :[[@LINE-4]]:27: note: at a call site, 'const struct T *' might bind with same force as 'struct T *'
+// CHECK-MESSAGES: :[[@LINE-5]]:49: note: at a call site, 'const struct T *' might bind with same force as 'struct T *'
+
+int equals(struct T a, struct T b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent parameters for 'equals' of similar type ('struct T') are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'a'
+// CHECK-MESSAGES: :[[@LINE-3]]:33: note: the last parameter in this range is 'b'
+
+typedef struct {
+  int x;
+} S;
+
+int equals2(S l, S r) { return l.x == r.x; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'equals2' of similar type ('S') are
+// CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in this range is 'l'
+// CHECK-MESSAGES: :[[@LINE-3]]:20: note: the last parameter in this range is 'r'