JonasToth added a comment. In https://reviews.llvm.org/D49114#1164788, @0x8000-0000 wrote:
> @aaron.ballman , @JonasToth , @Eugene.Zelenko Is there anything missing from > this patch? What do I need to do to get it merged? This is my first > contribution to LLVM so I'm not quite sure. Thank you! Usually the review process takes a while ;) If you have currently time and are motivated you can have multiple patches in parallel and add aaron, hokein, alexfh as reviewers. For the general procedure: - propose the patch - Review happens, discuss everything and fix problems - review gets accepted with a LGTM (looks good to me) + a green symbol in the review (alexfh or aaron.ballman are usually the reviewers that accept. You should usually wait for their go) - patch gets commited. You most likely don't have the commit access yet. But if you plan to contribute more often you can request it. For more information you can take a look at: https://llvm.org/docs/Contributing.html and https://llvm.org/docs/Phabricator.html and https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access ================ Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:42 + + const auto &Parents = Result.Context->getParents(*MatchedDecl); + for (const auto &Parent : Parents) { ---------------- 0x8000-0000 wrote: > Eugene.Zelenko wrote: > > Please don't use auto where type is not easily deducible. Same in other > > places. > This is the same code pattern as bugprone/MultipleStatementMacroCheck.cpp and > readability/RedundantDeclarationCheck.cpp. What type shall I use for Parent > instead? DynTypedNodeList::DynTypedNode ? Use the type the function returns (what auto would deduce to). ================ Comment at: clang-tidy/readability/MagicNumbersCheck.h:55 + const std::vector<std::string> IngnoredValuesInput; + std::vector<int64_t> IngnoredValues; +}; ---------------- Maybe these vectors could be `llvm::SmallVector` that has a better performance. ================ Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:11 + + double circleArea = 3.1415926535 * radius * radius; + ---------------- 0x8000-0000 wrote: > Eugene.Zelenko wrote: > > JonasToth wrote: > > > This example is good, but right now the code only checks for integer > > > literals. Maybe an integer example would be better? > > Please use .. code-block:: c++. Same for good example. > @JonasToth I raked my brain but I just can't come up with a short and > effective example. I haven't given up yet. There is an example in the cpp coreguidelines: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es45-avoid-magic-constants-use-symbolic-constants ``` for (int m = 1; m <= 12; ++m) // don't: magic constant 12 cout << month[m] << '\n'; ``` Maybe this could work? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
