Page MenuHomePhabricator

[clang-tidy] Zircon <fbl/limits.h> -> <limits>
AbandonedPublic

Authored by juliehockett on Nov 6 2018, 11:28 AM.

Details

Summary

Adds a check to convert <fbl/limits.h> to std <limits>.

This check is part of a set of migration checks as we prepare to move Zircon user code to use the C++ standard library, and should prevent regressions after the migration is complete.

Diff Detail

Event Timeline

juliehockett created this revision.Nov 6 2018, 11:28 AM
This revision is now accepted and ready to land.Nov 6 2018, 12:03 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
112

Please don't use auto because type could not be deduced from statement.

clang-tools-extra/docs/ReleaseNotes.rst
188

I think first statement is not necessary. Second should start from Suggests. Will be good idea to synchronize it with documentation.

190

Please highlight std with ``.

aaron.ballman added inline comments.Nov 6 2018, 1:24 PM
clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
48

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.)

71

Can users specialize fbl::numeric_limits for custom integral types like they can for std::numeric_limits? If so, that should maybe be covered with a checker as well.

72

::fbl instead of fbl to cover a situation like:

namespace oops {
namespace fbl {
struct numeric_limits {};
}

void foo() {
  fbl::numeric_limits n;
}
}
juliehockett marked 5 inline comments as done.
juliehockett added inline comments.
clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
48

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.

71

Technically, they could, but there are no instances of it in the codebase.

aaron.ballman added inline comments.Nov 6 2018, 6:48 PM
clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
48

Then why should this check be a public one, rather than an internal check?

juliehockett added inline comments.Nov 6 2018, 6:52 PM
clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
48

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.

aaron.ballman added inline comments.Nov 6 2018, 7:04 PM
clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
48

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?

juliehockett added inline comments.Nov 7 2018, 8:20 AM
clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
48

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?

aaron.ballman added inline comments.Nov 7 2018, 11:50 AM
clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
48

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.

This is https://bugs.llvm.org//show_bug.cgi?id=32739 . I think checks not relevant to a general audience (ie including the boost directory) should be in external plugins. It's not really clear to me what has to happen for that, but it seems increasingly relevant.

juliehockett abandoned this revision.Nov 12 2018, 10:26 AM

After a lot of discussion, we'll do this migration internally. Thanks for your comments!