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

[Whisperity via Phabricator via cfe-commits]

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());
+  StringRef CaptureName{NodeTypeName};
----------------
whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > 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.
> > > > 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?
> > > 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.
> > 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).
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69560/new/

https://reviews.llvm.org/D69560