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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
