[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity 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.
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560