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 parameters is not possible.
+ FB(Trivial, 2), //< The two mix trivially, and are the exact same type.
+ FB(Canonical, 3), //< The two mix because the types refer to the same
+ // CanonicalType, but we do not elaborate as to how.
+
----------------
alexfh wrote:
> whisperity wrote:
> > alexfh wrote:
> > > I'd switch to scoped enums (`enum class`), remove the macro, and use
> > > simpler code like `None = 0x1, Trivial = 0x2, Canonical = 0x4`, etc.
> > `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.
> 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.
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.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:133
+
+ void sanitize() {
+ assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");
----------------
alexfh wrote:
> whisperity wrote:
> > alexfh wrote:
> > > What should this method do and how it should be used? Same for
> > > Mix::sanitize()
> > 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.
> Thanks for the explanation! But it would be very helpful to have this
> information in the code, not only in a code review comment.
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.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:129
+
+void qualified1(int I, const int CI) {} // NO-WARN: Not the same type.
+
----------------
alexfh wrote:
> whisperity wrote:
> > alexfh wrote:
> > > Does the top-level const change anything with regard to the "mixability"?
> > > It's a purely implementation-side difference, callers could still swap
> > > parameters.
> > 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.
> 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.
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.
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
