whisperity marked an inline comment as done.
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:
> > > > > 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.
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
