[PATCH] D97297: [clang-tidy] Suppress reports to patternedly named parameters in 'bugprone-easily-swappable-parameters'

[Whisperity via Phabricator via cfe-commits]

whisperity marked an inline comment as not done.
whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:364-365
+                                                StringRef S2) {
+  StringRef S1Prefix = S1.take_front(S1.size() - N),
+            S2Prefix = S2.take_front(S2.size() - N);
+  return S1Prefix == S2Prefix && !S1Prefix.empty();
----------------
aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Should we be checking that `.size() - N` doesn't cause wraparound?
> > Wraparound as in underflow? Like `2 - 8` becoming multiple millions? The incoming strings are padded to the common length.
> > 
> > `take_xxx` won't let you go out of bounds, even if the parameter is greater than the string's length. It will just silently return the entire string.
> > Wraparound as in underflow? Like 2 - 8 becoming multiple millions? The incoming strings are padded to the common length.
> 
> They are, but I didn't see anything checking that `Threshhold` (which gets passed as `N`) isn't larger than the common string length.
> 
> > take_xxx won't let you go out of bounds, even if the parameter is greater than the string's length. It will just silently return the entire string.
> 
> Aha, that's the important bit, thanks for verifying that this is safe!
Nevertheless I'll clarify this, specifically because I have to make sure (I was thinking for a few minutes then realised the `!S1Prefix.empty()` is actually making sure of this...) that if you got parameter names of length 1 (let's say) and a threshold of 1, it really shouldn't say that `A` and `B` are "related" because the common prefix is `""` and the non-common end differs in one character, `A` vs. `B`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:384
+  // Pad the two strings to the longer length.
+  std::size_t BiggerLength = std::max(Str1.size(), Str2.size());
+  SmallString<32> S1PadE{Str1}, S2PadE{Str2};
----------------
@aaron.ballman I think I'll do something like `if (BiggerLength < Threshold) return false;`, how does that sound? The comparison is moot in that case, imagine having strings of 2 and 4, padded to 4, with a threshold of 6, for example.

That way even if someone accidentally makes `StringRef` overflow the buffer, we'll be safe on the call paths we have here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97297