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:
> > > > > > > > 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!)
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
