aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:47
+ SrcMgr::CharacteristicKind FileType) {
+ if (FileName == "fbl/limits.h") {
+ unsigned End = std::strcspn(SM.getCharacterData(HashLoc), "\n") + 1;
----------------
juliehockett wrote:
> aaron.ballman wrote:
> > juliehockett wrote:
> > > aaron.ballman wrote:
> > > > juliehockett wrote:
> > > > > aaron.ballman wrote:
> > > > > > Does this also work on platforms where the path separator is `\`
> > > > > > instead of `/`? What about case insensitive file systems where it
> > > > > > may be spelled `LiMiTs.H`? Does this properly handle a case like:
> > > > > > ```
> > > > > > #define LIMITS "fbl/limits.h"
> > > > > > #include LIMITS
> > > > > > ```
> > > > > > (Should add test cases for all of these scenarios.)
> > > > > Since this is a migration for our own codebase, we know we don't have
> > > > > any code that uses any variation other than <fbl/limits.h> and so
> > > > > hardcoding that is acceptable to us here.
> > > > Then why should this check be a public one, rather than an internal
> > > > check?
> > > I explained this on the other one, but for completeness:
> > >
> > >
> > > So yes, this check is for a migration, and we would delete it once
> > > regressions weren't possible. We would like the suite to be in upstream,
> > > however, because we use the ToT llvm/clang/tools/etc, and don't want to
> > > have to fork just to use clang-tidy for this sort of thing. Since
> > > clang-tidy doesn't provide any way to have external checks to the tool
> > > itself, upstreaming is the most ideal option.
> > >
> > > Orthogonal to our particular build setup, it'd also be nice to have an
> > > example of this sort of migration done by clang-tidy in-tree. There has
> > > been a lot of discussion recently about doing migrations with clang-tidy,
> > > but it's always describing an internal migration that uses a forked tree
> > > and a private suite of checks that can't be released.
> > I don't think it's reasonable to expect the community to bear the
> > maintenance burden for internal-only checks for an organization. I
> > definitely understand the desire not to carry around a fork of clang-tidy
> > for this, but that doesn't seem like a good reason for us to spend cycles
> > reviewing these patches, maintaining them, and then eventually removing
> > them.
> >
> > We have some precedent in that we have clang-tidy checks for llvm coding
> > standards or google coding standards, etc but those are also used outside
> > of the particular community for which they're named. In this case, I don't
> > think the functionality is useful to anyone except Google. Is that correct?
> That makes sense in terms of investment from you and others (though I do
> appreciate the effort you've put in for reviewing it thus far), but I (and my
> team) are willing to bear the burden of reviewing/maintaining/removing them.
> If clang-tidy provided a way to tack on a set of external checks without
> forking, we'd happily do that, but that's not really possible right now.
>
> Would it be reasonable to do the review/maintenance/removal of these
> migration checks from within our team, such that we're not asking you to
> spend cycles on them?
>
>
> Would it be reasonable to do the review/maintenance/removal of these
> migration checks from within our team, such that we're not asking you to
> spend cycles on them?
Personally, I don't think so. It still impose burdens on users of clang-tidy by
having checks that are not going to be useful to anyone except people from one
company. These checks appears in clang-tidy list checks, contribute to binary
sizes, are documented in user-facing places, users may enable them needlessly
and report bugs against them, etc.
I think that we shouldn't have any checks that are only useful for
(effectively) a single company -- I don't think that scales well, and I don't
think it benefits the community of users. I think a better approach is to
introduce plugins for clang-tidy checks. It's definitely a larger project than
what you're proposing and I understand if you don't want to tackle it, but it
benefits the entire community and solves the problem you want to solve.
https://reviews.llvm.org/D54169
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
