[PATCH] D142592: [clang-tidy][libc] Add an inline function checker for the libc project.

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp:27
+  // Consider only explicitly or implicitly inline functions.
+  if (!FuncDecl->isInlined())
+    return;
----------------
Check if FuncDecl is not a nullptr before dereferencing. You can do an early return like:

```
if (!FuncDecl)
  return;
```


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.h:27-36
+  InlineFunctionDeclCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
+            "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
+    if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
+                                    HeaderFileExtensions,
+                                    utils::defaultFileExtensionDelimiters())) {
----------------
Please implement constructor in the .cpp file.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.h:39-41
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
----------------
This is OK to keep in the header as it's only one line.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.h:43-51
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
+    Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+    if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
+                                    HeaderFileExtensions,
+                                    utils::defaultFileExtensionDelimiters())) {
+      this->configurationDiag("Invalid header file extension: '%0'")
+          << RawStringHeaderFileExtensions;
----------------
Ditto


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:100
 
+New `llvmlibc-inline-function-decl-check <http://clang.llvm.org/extra/clang-tidy/checks/llvmlibc/inline-function-decl-check.html>`_
+check.
----------------
I believe you'll need a rebase since a new check was recently added.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:104
+Checks that all implicit and explicit inline functions in header files are
+tagged with the ``LIBC_INLINE`` macro.
+
----------------
You need to also add the check to `clang-tools-extra/docs/clang-tidy/checks/list.rst`


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/llvmlibc/inline-function-decl.hpp:2
+// RUN: %check_clang_tidy %s llvmlibc-inline-function-decl-check %t \
+// RUN:   -- -header-filter=.* -- -I %S
+
----------------
I believe this can be removed.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/llvmlibc/inline-function-decl.hpp:20
+constexpr long long addll(long long a, long long b) {
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: 'addll' must be tagged with the LIBC_INLINE macro
+  return a + b;
----------------
Missing check name:

```
... LIBC_INLINE macro [llvmlibc-inline-function-decl]
```


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/llvmlibc/inline-function-decl.hpp:45
+  static int getVal(const MyClass &V) {
+  // CHECK-MESSAGES: warning: 'getVal' must be tagged with the LIBC_INLINE macro
+    return V.A;
----------------
Missing line and column info


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142592