[PATCH] D53771: [clang-tidy] Avoid C arrays check
Jonas Toth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 29 03:09:12 PDT 2018
JonasToth added inline comments.
================
Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:84
"cppcoreguidelines-c-copy-assignment-signature");
+ CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
+ "cppcoreguidelines-avoid-c-arrays");
----------------
please conserve the alphabetical order here
================
Comment at: clang-tidy/hicpp/HICPPTidyModule.cpp:54
"hicpp-avoid-goto");
+ CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
+ "hicpp-avoid-c-arrays");
----------------
i would this one should be before `avoid-goto`.
================
Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:44
+ unless(anyOf(hasParent(varDecl(isExternC())),
+ hasAncestor(functionDecl(isExternC())))))
+ .bind("typeloc"),
----------------
What about struct-decls that are externC?
================
Comment at: docs/clang-tidy/checks/modernize-avoid-c-arrays.rst:15
+observe *all* the uses of said declaration in order to decide whether it is
+safe to transform or not, and that is impossible in case of headers.
+
----------------
Not sure if we want the rational of not providing fixits here in this detail. Maybe it's better to formulate it as `fix-it are potentially dangerous in header files and are therefor not emitted right now`? Some places would be save to transform and we might want to provide these in the future.
================
Comment at: test/clang-tidy/modernize-avoid-c-arrays.cpp:48
+// Some header
+extern "C" {
+
----------------
Please add a test for extern c structs, maybe even in the form `extern "C" struct Foo { ... };`.
Additionally to that, maybe the opposite case should be tested too: `extern "C++"` in C-Code.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53771
More information about the cfe-commits
mailing list