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:
> > > > 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?
https://reviews.llvm.org/D54169
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
