aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this
function.
+
----------------
whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > whisperity wrote:
> > > > > aaron.ballman wrote:
> > > > > > whisperity wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > whisperity wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > I think this is a case where we could warn when the
> > > > > > > > > > declaration is outside of a system header (perhaps as an
> > > > > > > > > > option).
> > > > > > > > > >
> > > > > > > > > > Thinking about it a bit more, declarations and definitions
> > > > > > > > > > provide a novel way to get another kind of swap:
> > > > > > > > > > ```
> > > > > > > > > > void func(int x, int y);
> > > > > > > > > > void func(int y, int x) {} // Oops, the swap was probably
> > > > > > > > > > not intentional
> > > > > > > > > > ```
> > > > > > > > > > which may or may not be interesting for a check (or its
> > > > > > > > > > heuristics).
> > > > > > > > > I gave this some thought. It is a very good idea, but I
> > > > > > > > > believe not for //this// check, but D20689. What do you think
> > > > > > > > > of that? Maybe simply saying "call site v. function node that
> > > > > > > > > was called" could be extended with a completely analogous,
> > > > > > > > > string distance function based "function definition node v.
> > > > > > > > > redecl chain" case.
> > > > > > > > Hmmm, my impression is that this check is fundamentally about
> > > > > > > > *parameter* ordering whereas D20689 is about *argument*
> > > > > > > > ordering. Given that the example is about the ordering of
> > > > > > > > parameters and doesn't have anything to do with call sites, I
> > > > > > > > kind of think the functionality would be better in a
> > > > > > > > parameter-oriented check.
> > > > > > > >
> > > > > > > > That said, it doesn't feel entirely natural to add it to this
> > > > > > > > check because this check is about parameters that are easily
> > > > > > > > swappable *at call sites*, and this situation is not exactly
> > > > > > > > that. However, it could probably be argued that it is
> > > > > > > > appropriate to add it to this check given that swapped
> > > > > > > > parameters between the declaration and the definition are
> > > > > > > > likely to result in swapped arguments at call sites.
> > > > > > > >
> > > > > > > > Don't feel obligated to add this functionality to this check
> > > > > > > > (or invent a new check), though.
> > > > > > > It just seems incredibly easier to put it into the other check,
> > > > > > > as that deals with names. But yes, then it would be a bit weird
> > > > > > > for the "suspicious call argument" check to say something about
> > > > > > > the definition... This check (and the relevant Core Guideline)
> > > > > > > was originally meant to consider **only** types (this is
> > > > > > > something I'll have to emphasise more in the research paper
> > > > > > > too!). Everything that considers //names// is only for making the
> > > > > > > check less noisy and make the actually emitted results more
> > > > > > > useful. However, I feel we should //specifically// not rely on
> > > > > > > the names and the logic too much.
> > > > > > >
> > > > > > > But(!) your suggestion is otherwise a good idea. I am not sure if
> > > > > > > the previous research (with regards to names) consider the whole
> > > > > > > "forward declaration" situation, even where they did analysis for
> > > > > > > C projects, not just Java.
> > > > > > > However, I feel we should specifically not rely on the names and
> > > > > > > the logic too much.
> > > > > >
> > > > > > I agree, to a point. I don't think the basic check logic should be
> > > > > > name-sensitive, but I do think we need to rely on names to tweak
> > > > > > the true/false positive/negative ratios. I think most of the time
> > > > > > when we're relying on names should wind up being configuration
> > > > > > options so that users can tune the algorithm to their needs.
> > > > > >
> > > > > > We could put the logic into the suspicious call argument check with
> > > > > > roughly the same logic -- the call site looks like it has
> > > > > > potentially swapped arguments because the function redeclaration
> > > > > > chain (which includes the function definition, as that's also a
> > > > > > declaration) has inconsistent parameter names. So long as the
> > > > > > diagnostic appears on the call site, and then refers back to the
> > > > > > inconsistent declarations, it could probably work. However, I think
> > > > > > it'd be a bit strange to diagnose the call site because the user of
> > > > > > the API didn't really do anything wrong (and they may not even be
> > > > > > able to change the declaration or the definition where the problem
> > > > > > really lives).
> > > > > But wait... suppose there is a project A, which sees the header for
> > > > > `f(int x, int y)` and has a call `f(y, x)`. That will be diagnosed.
> > > > > But if project A is only a client of `f`, it should be highly
> > > > > unlikely that project A has the //definition// of `f` in their tree,
> > > > > right? So people of A will not get the warning for that. Only what is
> > > > > part of the compilation process will be part of the analysis process.
> > > > >
> > > > > Similarly to how this check matches **only** function definitions,
> > > > > and never a prototype. The latter one would have bazillion of
> > > > > warnings from every header ever, with the user not having a chance to
> > > > > fix it anyways.
> > > > > But wait... suppose there is a project A
> > > >
> > > > That's the point I was trying to make about it being strange to
> > > > diagnose on the call site -- the user of the API didn't use it wrong,
> > > > the designer of the API did something bad.
> > > >
> > > > It's sounding more like this functionality is a good idea for a new
> > > > check rather than an existing check.
> > > Okay, now I understand! But I did not intend to diagnose it on the call
> > > site. The other check itself would share the same configuration
> > > parameters and decision-making for string distance, and the "fwd decl vs.
> > > definition" diagnostic would come emitted to the definition's location!
> > > And the "call site vs. called function" would be diagnosed at the call
> > > site. And if there isn't a definition in the analysed TU for some
> > > function, the former logic is simply skipped.
> > > But I did not intend to diagnose it on the call site.
> >
> > I'd find that to be a bit strange given that the other check is called
> > `readability-suspicious-call-argument`, because there would be no call in
> > that situation. Or am I misunderstanding? (Sorry if I'm being dense!)
> No, it //is// strange! And I think we will come up with a better idea for
> this because the check logic itself sounds very plausible, and needed. Either
> factor out the string distance logic somewhere and have two checks with
> separate matchers, but the same underlying logic then, or rename the other
> check... The two diagnostics would have a different message either way. The
> latter option (finding a new name for the check) sounds like the path of
> least resistance.
Ah, yes! I think we're on the same page now. :-)
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
