[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:34
+    std::is_base_of<Redeclarable<T>, T>::value
+        &&std::is_base_of<NamedDecl, T>::value;
+
----------------



================
Comment at: clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:84
+    if (!Check.checkCorrespongdingHeader())
+      return; // Found a good candidate for matching decl
+    StringRef ThisStem = path::stem(SM.getFilename(BeginLoc));
----------------



================
Comment at: clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:87
+    if (!ThisStem.empty() && Stem.startswith_lower(ThisStem))
+      return; // Found a good candidate for matching decl
+  }
----------------



================
Comment at: clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:162
+
+static NameAndFixMode getNameAndFixMode(TagTypeKind Kind, bool CPlusPlus) {
+  FixMode Mode = CPlusPlus ? FixMode::AnonNamespace : FixMode::None;
----------------
clang-tidy diagnostics do not start with a capital letter, so these string literals should all be lowercase.


================
Comment at: clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:190
+                                  utils::defaultFileExtensionDelimiters())) {
+    llvm::errs() << "Invalid implementation file extension: "
+                 << RawStringImplementationFileExtensions << "\n";
----------------
Should this be using `configurationDiag()` instead of `llvm::errs()`?


================
Comment at: clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:232-234
+    checkDecl(*this, *VD, *Result.Context, {"Variable", FixMode::Static});
+  else if (const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("FD"))
+    checkDecl(*this, *FD, *Result.Context, {"Function", FixMode::Static});
----------------
These string literals should also be lowercased.


================
Comment at: clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:253
+  // Disable running this check on a file that isn't classed as an
+  // implementation file. can occur when running in clangd.
+  if (!isImplementationFile(getCurrentMainFile()))
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-missing-header-file-declaration.cpp:12
+extern bool DeclInSource;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Variable 'DeclInSource' is declared as extern in a source file
+// CHECK-FIXES: {{^\s*}}bool DeclInSource;
----------------
Warning on this construct worries me -- it's not uncommon to use extern declarations in some language modes. For instance, in C, you'll still find plenty of older code bases that use an extern declaration of a system function rather than `#include`'ing the standard library header. Also, folks will use extern definitions as a way to smuggle data between TUs without exposing a public interface (not always the best of practices, but it is an approach to hiding implementation details).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73413/new/

https://reviews.llvm.org/D73413