This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy][libc] Add an inline function checker for the libc project.
ClosedPublic

Authored by sivachandra on Jan 25 2023, 5:34 PM.

Details

Summary

The new checker checks if inline functions defined in header files are
tagged with the LIBC_INLINE macro. See https://libc.llvm.org/code_style.html
for more information about this macro.

Diff Detail

Event Timeline

sivachandra created this revision.Jan 25 2023, 5:34 PM
sivachandra requested review of this revision.Jan 25 2023, 5:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2023, 5:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Please add entry in Release Notes.

clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp
30

Please don't use auto unless type is explicitly stated in same statement or iterator.

Use explicit type instead of auto.

sivachandra marked an inline comment as done.Jan 25 2023, 9:59 PM
PiotrZSL added inline comments.
clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp
32

Probably better would be to check if this isn't main source Result.SourceManager->isInMainFile(SrcBegin)

36

Maybe put this LIBC_INLINE into some configuration, so check could be used by someone else also for other macros.

PiotrZSL added inline comments.Jan 26 2023, 1:50 AM
clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp
21

or maybe even better:
Finder->addMatcher(functionDecl(unless(isExpansionInMainFile()), isInline()).bind("func_decl"), this);
Instead of line 26 and 32.

carlosgalvezp added inline comments.Jan 26 2023, 6:36 AM
clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp
21

I'm not sure that works - if we pass a header directly to clang-tidy, it will consider it as the "main file", right? That's why the established pattern is based on HeaderFileExtensions, please check out other checks.

30

We have a well-established pattern for detecting code in headers, grep for HeaderFileExtensions in existing checks.

PiotrZSL added inline comments.Jan 26 2023, 7:26 AM
clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp
21

Yes you right, but isInline still can be checked here.
As for HeaderFileExtensions never used it from both developer and user point of view.
When running clang-tidy on headers is still better simply create file with single include.
Maybe if there would be AST_MATCHER for HeaderFileExtensions.

carlosgalvezp added inline comments.Jan 26 2023, 8:11 AM
clang-tools-extra/test/clang-tidy/checkers/llvmlibc/inline-function-decl.cpp
18–19 ↗(On Diff #492324)

These should specify which line they apply to, as done in all other checks.

In particular, to get the unit test to work, you will not be able to use a .cpp that includes a header. The test file must be a .hpp file (for which you need HeaderFileExtensions as I said above).

Use the HeaderFileExtensions pattern to match constructs in header files.

Update release notes.

Eugene.Zelenko added inline comments.Jan 26 2023, 4:44 PM
clang-tools-extra/docs/ReleaseNotes.rst
104

One sentence is enough.

sivachandra added inline comments.Jan 26 2023, 4:47 PM
clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp
21

Unfortunately, even isInline is not good enough because isInline only matches functions marked with inline: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/ASTMatchers/ASTMatchers.h#L7769. It misses implicitly inline functions like constexpr functions and class methods.

30

Thanks! I have now switched over to use the HeaderFileExtensions pattern.

sivachandra marked an inline comment as done.

Limit the description in the release notes to one sentence.

Limit the description in the release notes to one sentence.

carlosgalvezp added inline comments.Feb 4 2023, 8:12 AM
clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp
27

Check if FuncDecl is not a nullptr before dereferencing. You can do an early return like:

if (!FuncDecl)
  return;
clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.h
27–36

Please implement constructor in the .cpp file.

39–41

This is OK to keep in the header as it's only one line.

43–51

Ditto

clang-tools-extra/docs/ReleaseNotes.rst
100

I believe you'll need a rebase since a new check was recently added.

104

You need to also add the check to clang-tools-extra/docs/clang-tidy/checks/list.rst

clang-tools-extra/test/clang-tidy/checkers/llvmlibc/inline-function-decl.hpp
2

I believe this can be removed.

20

Missing check name:

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

Missing line and column info

carlosgalvezp added inline comments.Feb 4 2023, 8:58 AM
clang-tools-extra/docs/clang-tidy/checks/llvmlibc/inline-function-decl-check.rst
8

Document the HeaderFileExtensions option, see existing checks.

carlosgalvezp added inline comments.Feb 4 2023, 9:01 AM
clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
28

Remove

clang-tools-extra/docs/ReleaseNotes.rst
100

Remove

100

Remove

clang-tools-extra/docs/clang-tidy/checks/llvmlibc/inline-function-decl-check.rst
1

Remove

3

Remove

4

Nit: remove extra =

sivachandra marked 14 inline comments as done.

Address comments.

carlosgalvezp accepted this revision.Feb 6 2023, 11:25 PM

LGTM, thanks for the contribution! Please wait a few days for other reviewers to have a look.

This revision is now accepted and ready to land.Feb 6 2023, 11:25 PM