PiotrZSL accepted this revision.
PiotrZSL added a comment.
This revision is now accepted and ready to land.

From functional point of view it's looking good.



================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp:42
 
+  // Consider only functions with an external and visible declaration.
+  if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl)) {
----------------
NOTE: This comment refers also to inline exclusion in line 33.
I don't think is needed, or you can extract line 33 and 43 into separate 
function like isExternalyVisibleDeclaration, and to be honest for check that 
there are other ways, some checks already do that, because simply then you can 
ask your self, what about coonstexpr functions, what about static functions, 
functions in anonymous namespace In theory they also may be catch here. Even 
that inline keyword could be redundant.



================
Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvmlibc/inline-function-decl.hpp:66
+// CHECK-MESSAGES-NOT: :[[@LINE+3]]:3: warning: 'operator void (*)()' must be 
tagged with the LIBC_INLINE macro; the macro should be placed at the beginning 
of the declaration [llvmlibc-inline-function-decl]
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:3: warning: '~(lambda at [[FILENAME:.+]])' 
must be tagged with the LIBC_INLINE macro; the macro should be placed at the 
beginning of the declaration [llvmlibc-inline-function-decl]
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:6: warning: 'operator()' must be tagged 
with the LIBC_INLINE macro; the macro should be placed at the beginning of the 
declaration [llvmlibc-inline-function-decl]
----------------
NOTE: If this warning were printed then we got other issue in this check.
Implicit functions are checked, would be good to exclude them:
`functionDecl(unless(isImplicit()))`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148444

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to