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

whisperity marked 3 inline comments as done. whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:57 + "Const_Reverse_Iterator", +

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

RKSimon added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:57 + "Const_Reverse_Iterator", + "const_reverse_iterator" +

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

whisperity marked 2 inline comments as done. whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:113-120 +#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull)) + + FB(None, 1), //< Mix between the two

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

alexfh added a comment. I thought I sent this batch of comments loong ago =\ Better late than never. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:113-120 +#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull)) + + FB(None, 1),

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG499e39c5983d: [clang-tidy] Add bugprone-easily-swappable-parameters check (authored by whisperity). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69560/new/

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

aaron.ballman added a comment. 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

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

whisperity added a comment. 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

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

aaron.ballman accepted this revision. aaron.ballman added a comment. 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

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

whisperity updated this revision to Diff 354166. whisperity added a comment. 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). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

whisperity updated this revision to Diff 353983. whisperity added a comment. (Try nudging the CI to only apply this commit after removal of parent links.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69560/new/ https://reviews.llvm.org/D69560

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

whisperity added a comment. 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

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

whisperity updated this revision to Diff 353969. whisperity marked 8 inline comments as done. whisperity added a comment. 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

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

whisperity added a comment. 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. Repository: rG LLVM Github

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

whisperity added a comment. 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

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

alexfh added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

alexfh added a comment. 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. Comment at:

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

whisperity updated this revision to Diff 336622. whisperity added a comment. Made sure that the parameter's name is highlighted promptly in the //"first|last parameter of the range is..."// note. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

whisperity updated this revision to Diff 336618. whisperity set the repository for this revision to rG LLVM Github Monorepo. whisperity added a comment. Rebased over D98635 , allowing us to properly highlight the found //"mixable adjacent parameter range"//

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:363 +/// Returns the diagnostic-friendly name of the node, or empty string. +static SmallString<64> getName(const NamedDecl *ND) { + SmallString<64> Name;

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:363 +/// Returns the diagnostic-friendly name of the node, or empty string. +static SmallString<64> getName(const NamedDecl *ND) { + SmallString<64> Name;

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

whisperity updated this revision to Diff 331241. whisperity marked 6 inline comments as done. whisperity added a subscriber: Szelethus. whisperity added a comment. - Add `const reverse iterator` to the default-ignored typename list. CHANGES SINCE LAST ACTION

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

aaron.ballman added a comment. 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

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

whisperity added a comment. 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

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. 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

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:363 +/// Returns the diagnostic-friendly name of the node, or empty string. +static SmallString<64> getName(const NamedDecl *ND) { + SmallString<64> Name;

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56 + +#ifndef NDEBUG + aaron.ballman wrote: > Are you planning to remove the debugging code now that the check is > approaching its final

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:54 + "reverse_iterator", + "reverse_const_iterator"}); +

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

whisperity added a comment. @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

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

whisperity updated this revision to Diff 326439. whisperity added a comment. - **[NFC]** Reformat the documentation so option values are in single backticks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69560/new/ https://reviews.llvm.org/D69560

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

whisperity updated this revision to Diff 323305. whisperity added a comment. Make sure that overloadable binary operators are ignored properly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69560/new/ https://reviews.llvm.org/D69560 Files:

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

whisperity updated this revision to Diff 322363. whisperity added a comment. **NFC** (With all the restructuring and trying to figure out how to implement the latest change, I managed to upload the wrong, unclean diff...) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

whisperity marked an inline comment as done. whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255 + std::string NodeTypeName = + Node->getType().getAsString(Node->getASTContext().getPrintingPolicy()); +

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

whisperity updated this revision to Diff 322357. whisperity added a comment. Made detection of macros as "parameter type names" more robust. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69560/new/ https://reviews.llvm.org/D69560 Files:

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

whisperity marked 4 inline comments as done. whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255 + std::string NodeTypeName = + Node->getType().getAsString(Node->getASTContext().getPrintingPolicy()); +

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

whisperity updated this revision to Diff 322339. whisperity added a comment. - 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. -

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

whisperity added a comment. 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

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

steveire added a comment. 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

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177 + const std::size_t StartIndex) { + const std::size_t NumParams = FD->getNumParams(); + assert(StartIndex <

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177 + const std::size_t StartIndex) { + const std::size_t NumParams = FD->getNumParams(); + assert(StartIndex <

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177 + const std::size_t StartIndex) { + const std::size_t NumParams = FD->getNumParams(); + assert(StartIndex <

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

riccibruno added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:337 + if (Name.empty()) +Name = ""; + return Name; I think that this will become unreachable after D84658 and D85033. I may be missing

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

whisperity marked 6 inline comments as done. whisperity added inline comments. Herald added a subscriber: nullptr.cpp. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177 + const std::size_t StartIndex)

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10 + +void declaration(int Param, int Other); // NO-WARN: No chance to change this function. + whisperity wrote: >

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

whisperity added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10 + +void declaration(int Param, int Other); // NO-WARN: No chance to change this function. + aaron.ballman wrote: >

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10 + +void declaration(int Param, int Other); // NO-WARN: No chance to change this function. + whisperity wrote: >

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

whisperity added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10 + +void declaration(int Param, int Other); // NO-WARN: No chance to change this function. + aaron.ballman wrote: >

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10 + +void declaration(int Param, int Other); // NO-WARN: No chance to change this function. + whisperity wrote: >

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403 + +// Unfortunately, undersquiggly highlights are for some reason chewed up +// and not respected by diagnostics from Clang-Tidy. :( +

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

whisperity marked an inline comment as done. whisperity added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10 + +void declaration(int Param, int Other); // NO-WARN: No chance to change this function. +

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

whisperity added a comment. 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`

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403 + +// Unfortunately, undersquiggly highlights are for some reason chewed up +// and not respected by diagnostics from Clang-Tidy. :( +

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

steveire added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

whisperity marked 3 inline comments as done. whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403 + +// Unfortunately, undersquiggly highlights are for some reason chewed up +// and not respected

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

whisperity marked 6 inline comments as done. whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255 + std::string NodeTypeName = + Node->getType().getAsString(Node->getASTContext().getPrintingPolicy()); +

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:58 + +namespace { + whisperity wrote: > aaron.ballman wrote: > > Is there a need for the anonymous namespace? (We typically only use them

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:284 +/// The default value for the MinimumLength check option. +static constexpr unsigned DefaultMinimumLength = 2; + aaron.ballman wrote: >

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

whisperity updated this revision to Diff 319840. whisperity marked 9 inline comments as done. whisperity added a comment. - NFC Code style and organisation fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69560/new/

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

whisperity added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10 + +void declaration(int Param, int Other); // NO-WARN: No chance to change this function. + aaron.ballman wrote: > I think

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:58 + +namespace { + aaron.ballman wrote: > Is there a need for the anonymous namespace? (We typically only use them when > defining a class

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:58 + +namespace { + Is there a need for the anonymous namespace? (We typically only use them when defining a class and try to make them as

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

whisperity updated this revision to Diff 319315. whisperity added a comment. - 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 `typedef`s) - Fixed Clang-Format and Clang-Tidy

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

whisperity updated this revision to Diff 318220. whisperity retitled this revision from "[clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check" to "[clang-tidy] Add 'bugprone-easily-swappable-parameters' check". whisperity edited the summary of this