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