aaron.ballman added a comment. In D69560#1890767 <https://reviews.llvm.org/D69560#1890767>, @vingeldal wrote:
> In D69560#1890026 <https://reviews.llvm.org/D69560#1890026>, @aaron.ballman > wrote: > > > That is an option, but it would be better for the check to be usable > > without requiring a lot of custom configuration. > > > One can't really expect a ones own .clang-tidy file to be applicable to all > third party code used in ones project. It is a fact, with or without this > check, that if you use third party libraries you can't expect the third party > libraries to comply with your own .clang-tidy file. You should expect to set > up a separate .clang-tidy file for all third party libraries used by your > project with or without this check -and that's ok because it is arguably not > a big effort to do so. We typically aim for checks to require as little configuration as possible by tailoring the default check behavior to be as reasonable as we can make it. >> Incremental improvement is always a good thing, but it has to start from a >> stable base. I'm not yet convinced this check meets the bar for inclusion in >> clang-tidy. There is no proposed way to tell how two parameters relate to >> one another, and the simple enforcement seems dangerously simple. >> >>> Would it not be a reasonable approach to accept this implementation and >>> improve `NOLINT` to work on entire files or lists of files rather than >>> squeezing more heuristics (prone to false negatives) to the check? >> >> AFAICT, that would be a novel new use of `NOLINT`, so I'm wary of it. I >> think the first thing is to gather some data as to how this check performs >> as-is. If the behavior is unreasonable, we can see what ways in which it is >> unreasonable with the dataset and see if we can tease out either heuristics >> or configuration options that would make it more palatable. We may also want >> to rope some of the C++ Core Guidelines authors in on the discussion at that >> point because that would be data suggesting their suggested simple >> enforcement is untenable. > > There is also the --header-filter command line option to configure the tool > to skip some headers. That is true. > To sum up my position: > I'm all for investigating the performance of this check and basing any > decisions on data. Until we have that data I just want to challenge the claim > that there are no suitable ways to turn the check of for select portions of > the code base, > there are definitely ways to do that which both work satisfyingly and are in > no way novel or unconventional. Using the header filters would be a reasonable approach, if a bit heavy-handed. Ideally, the check would warn about what the rule wants it to warn about. I think the unsatisfying bit is the lack of clarity in the rule itself. Trying to write a checker for a rule with that much subjectivity typically leads to a lot of configuration needs. In D69560#1890855 <https://reviews.llvm.org/D69560#1890855>, @whisperity wrote: > In D69560#1889925 <https://reviews.llvm.org/D69560#1889925>, @vingeldal wrote: > > > I think this value very well may strike a good balance in that compromise > > but I still think it is a clear deviation from the guideline as specified. > > IMO all deviations from the guideline should be explicitly requested by > > the user, not set by default, no matter how useful that deviation is. > > > While I am still planning to compile a measurement with potentially tagging > false and true positives -- albeit classifying this on random projects I have > no domain knowledge about is hard at first glance... LLVM itself might be a good starting point since we've all got some passing familiarity with the project. > @aaron.ballman Could it be a useful compromise to //not// say we are trying > to solve the `cppcoreguidelines-` rule, but rather introduce this checker in > another "namespace" and maybe mention in the docs that it could, although > with caveats, apply "parts" of the Core Guidelines rule? I think that could be a useful compromise, or name it `cppcoreguidelines-experimental-avoid-adjacent-parameters-of-same-type` to make it clear that we're trying things out to see what works in practice and the behavior may not match the guideline. I think the most important thing we do will be to eventually coordinate with the C++ Core Guidelines folks though. The eventual goal is for the clang-tidy check to cover what the guideline wants it to cover as its default behavior, but in a way that's going to give acceptable behavior for our users. This may mean we have a slightly chattier diagnostic than I'd like, and it may also mean that some updates to the core guideline are needed to help clarify what constitutes a "related" parameter in a way that's acceptable for analysis tools. What I want to avoid is having users say "this check doesn't match the guideline" or "we want to enable all the guidelines but we always have to disable this one because of the false positives". Btw, we should update the terminology in the patch to use `parameter` instead of `argument` (parameters are what the function declares, arguments are what are passed in at the call site and they bind to parameters -- the issue this check is trying to solve is on the declaration side, not the caller side). 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
