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 and try to make them as narrow in scope as possible, and use
> `static` for function declarations instead.)
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?
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:67
+/// The language features involved in allowing the mix between two parameters.
+enum MixFlags : unsigned char {
+ MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE!
----------------
aaron.ballman wrote:
> 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.
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.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:90-91
+
+ void sanitise() {
+ assert(Flags != MIX_Invalid && "sanitise() called on invalid bitvec");
+ // TODO: There will be statements here in further extensions of the check.
----------------
aaron.ballman wrote:
> Heh, I don't know if we have any guidance on US English vs British English
> spellings. ;-)
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.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+ std::string NodeTypeName =
+ Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+ StringRef CaptureName{NodeTypeName};
----------------
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?
================
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. :(
+ diag(First->getLocation(), "the first parameter in the range is '%0'",
----------------
aaron.ballman wrote:
> Can you post a screenshot of what you mean?
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 [[
http://lists.llvm.org/pipermail/cfe-dev/2019-October/063676.html | the relevant
mailing list post ]].
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:30
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+ /// The minimum length of an adjacent swappable parameter range required for
----------------
aaron.ballman wrote:
> Do these data members need to be public?
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);
```
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:14
+
+S *allocate() { return nullptr; } // NO-WARN: 1
parameter.
+void allocate(S **Out) {} // NO-WARN: 1
parameter.
----------------
aaron.ballman wrote:
>
Good catch! This is what you get when you copy-paste. 😁
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69560/new/
https://reviews.llvm.org/D69560
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
