Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline
Changeset View
Standalone View
clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp
- This file was added.
| //===-- InlineFunctionDeclCheck.cpp ---------------------------------------===// | |||||
| // | |||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | |||||
| // See https://llvm.org/LICENSE.txt for license information. | |||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||||
| // | |||||
| //===----------------------------------------------------------------------===// | |||||
| #include "InlineFunctionDeclCheck.h" | |||||
| #include "clang/AST/ASTContext.h" | |||||
| #include "clang/ASTMatchers/ASTMatchFinder.h" | |||||
| #include "llvm/ADT/StringSet.h" | |||||
| using namespace clang::ast_matchers; | |||||
| namespace clang::tidy::llvm_libc { | |||||
| void InlineFunctionDeclCheck::registerMatchers(MatchFinder *Finder) { | |||||
| Finder->addMatcher(decl(functionDecl()).bind("func_decl"), this); | |||||
| } | |||||
PiotrZSL: or maybe even better:
Finder->addMatcher(functionDecl(unless(isExpansionInMainFile()), isInline… | |||||
Not Done ReplyInline ActionsI'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. carlosgalvezp: I'm not sure that works - if we pass a header directly to clang-tidy, it will consider it as… | |||||
Not Done ReplyInline ActionsYes you right, but isInline still can be checked here. PiotrZSL: Yes you right, but isInline still can be checked here.
As for HeaderFileExtensions never used… | |||||
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. sivachandra: Unfortunately, even `isInline` is not good enough because `isInline` only matches functions… | |||||
| void InlineFunctionDeclCheck::check(const MatchFinder::MatchResult &Result) { | |||||
| const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("func_decl"); | |||||
| // 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; carlosgalvezp: Check if FuncDecl is not a nullptr before dereferencing. You can do an early return like:
```… | |||||
| SourceLocation SrcBegin = FuncDecl->getBeginLoc(); | |||||
Please don't use auto unless type is explicitly stated in same statement or iterator. Eugene.Zelenko: Please don't use `auto` unless type is explicitly stated in same statement or iterator. | |||||
Not Done ReplyInline ActionsWe have a well-established pattern for detecting code in headers, grep for HeaderFileExtensions in existing checks. carlosgalvezp: We have a well-established pattern for detecting code in headers, grep for… | |||||
Thanks! I have now switched over to use the HeaderFileExtensions pattern. sivachandra: Thanks! I have now switched over to use the `HeaderFileExtensions` pattern. | |||||
| // Consider functions only in header files. | |||||
| if (!utils::isSpellingLocInHeaderFile(SrcBegin, *Result.SourceManager, | |||||
Not Done ReplyInline ActionsProbably better would be to check if this isn't main source Result.SourceManager->isInMainFile(SrcBegin) PiotrZSL: Probably better would be to check if this isn't main source Result.SourceManager->isInMainFile… | |||||
| HeaderFileExtensions)) | |||||
| return; | |||||
| // Check if decl starts with LIBC_INLINE | |||||
Not Done ReplyInline ActionsMaybe put this LIBC_INLINE into some configuration, so check could be used by someone else also for other macros. PiotrZSL: Maybe put this LIBC_INLINE into some configuration, so check could be used by someone else also… | |||||
| auto Loc = FullSourceLoc(Result.SourceManager->getFileLoc(SrcBegin), | |||||
| *Result.SourceManager); | |||||
| llvm::StringRef SrcText = Loc.getBufferData().drop_front(Loc.getFileOffset()); | |||||
| if (SrcText.starts_with("LIBC_INLINE")) | |||||
| return; | |||||
| diag(SrcBegin, "%0 must be tagged with the LIBC_INLINE macro; the macro " | |||||
| "should be placed at the beginning of the declaration") | |||||
| << FuncDecl; | |||||
| } | |||||
| } // namespace clang::tidy::llvm_libc | |||||
or maybe even better:
Finder->addMatcher(functionDecl(unless(isExpansionInMainFile()), isInline()).bind("func_decl"), this);
Instead of line 26 and 32.