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:
> > > > > > > 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.
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
