[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

alexfh added a comment. A couple of late comments. Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:39 + return; +} else if (DefaultArgRange.getBegin().isMacroID()) { + diag(D->getLocStart(), No `else` after `return`, please.

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added a comment. In https://reviews.llvm.org/D40108#938131, @juliehockett wrote: > If you could, that'd be great! Thank you! It'll be good idea it you'll request commit rights, since you have more checks in development queue. https://reviews.llvm.org/D40108

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r319225. https://reviews.llvm.org/D40108 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett added a comment. If you could, that'd be great! Thank you! https://reviews.llvm.org/D40108 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

aaron.ballman added a comment. Thanks for the nit fix. If you'd like me to commit this on your behalf, just let me know. https://reviews.llvm.org/D40108 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from one small nit with the test file missing a trailing newline. Comment at: test/clang-tidy/fuchsia-default-arguments.cpp:82 +} \ No newline at

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett updated this revision to Diff 124421. juliehockett marked an inline comment as done. juliehockett added a comment. Updating docs https://reviews.llvm.org/D40108 Files: clang-tidy/CMakeLists.txt clang-tidy/fuchsia/CMakeLists.txt clang-tidy/fuchsia/DefaultArgumentsCheck.cpp

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added inline comments. Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:6 + +Warns if a function is declared or called with default arguments. + Please synchronize with text in Release Notes. https://reviews.llvm.org/D40108

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett updated this revision to Diff 124397. juliehockett marked 7 inline comments as done. juliehockett added a comment. Added new tests and updated wording in warning. https://reviews.llvm.org/D40108 Files: clang-tidy/CMakeLists.txt clang-tidy/fuchsia/CMakeLists.txt

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:35-38 +SourceRange RemovalRange( + Lexer::getLocForEndOfToken(D->getLocation(), 0, +*Result.SourceManager, Result.Context->getLangOpts()), +

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett updated this revision to Diff 123857. juliehockett marked 6 inline comments as done. juliehockett added a comment. Disabled the fix-it for `getDefaultArgRange()` that returns an empty range and for default args that come from a macro. https://reviews.llvm.org/D40108 Files:

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett added a comment. We're going to go with fuchsia-* for the module name, since the style applies to the broader project. Also, there may be zircon-specific checks at some point, so we want to leave the door open for that. Comment at:

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

aaron.ballman added a comment. Did you decide on fuchsia instead of zircon for the module name, or is that decision still up in the air? Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:22 + // Calling a function which uses default arguments is disallowed. +

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett updated this revision to Diff 123677. juliehockett marked 8 inline comments as done. juliehockett added a comment. Moved AST matcher out to ASTMatchers.h (see D40261 ), fixing comments https://reviews.llvm.org/D40108 Files:

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett added inline comments. Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:22 + // Calling a function which uses default arguments is disallowed. + Finder->addMatcher(cxxDefaultArgExpr().bind("stmt"), this); + // Declaring default parameters is disallowed.

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Thank you for the explanation about what is driving this and the documentation. I don't think google-* is the correct module for this as it's too general of an

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:63 + + Warns if a function is declared or called with default arguments. + I think will be good idea to change to //function// to //function or method//. Same in documentation.

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett added a comment. In https://reviews.llvm.org/D40108#928224, @Eugene.Zelenko wrote: > > I think it should use project-specific prefix, since it's open source > project. Google may have different coding guidelines for other projects. Reasonable. It makes sense to consider it on

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett updated this revision to Diff 123446. juliehockett marked 2 inline comments as done. juliehockett added a comment. Updated docs and added tests to check class methods. https://reviews.llvm.org/D40108 Files: clang-tidy/CMakeLists.txt clang-tidy/fuchsia/CMakeLists.txt

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added a comment. In https://reviews.llvm.org/D40108#928212, @juliehockett wrote: > We do need to figure out what the right prefix for these checks is (whether > fuchsia-* since they'll be used under the Fuchsia umbrella, zircon-* since > the follow the Zircon section of the

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added a comment. Does Fuchsia coding conventions allows default parameters in class methods? If not, test should reflect this Comment at: docs/ReleaseNotes.rst:63 + + Prevent use of default arguments in declared or called functions in Fuchsia. +

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett marked 2 inline comments as done. juliehockett added a comment. We do need to figure out what the right prefix for these checks is (whether fuchsia-* since they'll be used under the Fuchsia umbrella, zircon-* since the follow the Zircon section of the style guide linked above, or

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett updated this revision to Diff 123278. juliehockett edited the summary of this revision. juliehockett added a comment. Updated docs https://reviews.llvm.org/D40108 Files: clang-tidy/CMakeLists.txt clang-tidy/fuchsia/CMakeLists.txt clang-tidy/fuchsia/DefaultArgumentsCheck.cpp

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:60 + Prevent use of default arguments in declared or called functions in Fuchsia. + Check name and link was accidentally removed. Comment at:

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett updated this revision to Diff 123231. juliehockett marked an inline comment as done. juliehockett edited the summary of this revision. https://reviews.llvm.org/D40108 Files: clang-tidy/CMakeLists.txt clang-tidy/fuchsia/CMakeLists.txt

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

aaron.ballman added a comment. For background: what is Fuchsia and where do these requirements come from (are they documented publicly somewhere)? We tend to prefer concise patches over code dumps, so I think it would make sense to split this review into multiple patches. The first one can be

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added a comment. Please mention new module in docs/clang-tidy/index.rst. Repository: rL LLVM https://reviews.llvm.org/D40108 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added inline comments. Comment at: clang-tidy/fuchsia/FuchsiaTidyModule.cpp:31 + void addCheckFactories(ClangTidyCheckFactories ) override { + +CheckFactories.registerCheck( Please remove empty line. Comment at:

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

Eugene.Zelenko added a comment. Please split this review with one check per review. Comment at: docs/ReleaseNotes.rst:83 + + Check to prevent creation of statically-stored objects in Fuchsia. + Please fix double space Comment at:

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

juliehockett created this revision. juliehockett added a project: clang-tools-extra. Herald added subscribers: xazax.hun, mgorny. This adds a Fuchsia module to clang-tidy to warn for features that are disallowed. The following checks were added to the new module: fuchsia-default-arguments: