[PATCH] D54169: [clang-tidy] Zircon <fbl/limits.h> -> <limits>

[Julie Hockett via Phabricator via cfe-commits]

juliehockett 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;
----------------
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.


https://reviews.llvm.org/D54169