Page MenuHomePhabricator

[libc][clang-tidy] fix namespace check for externals
ClosedPublic

Authored by michaelrj on Nov 15 2021, 3:15 PM.

Details

Summary

Up until now, all references to errno were marked with NOLINT, since
it was technically calling an external function. This fixes the lint
rules so that errno, as well as malloc, calloc, realloc, and
free are all allowed to be called as external functions. All of the
relevant NOLINT comments have been removed, and the documentation has
been updated.

Diff Detail

Event Timeline

michaelrj created this revision.Nov 15 2021, 3:15 PM
michaelrj requested review of this revision.Nov 15 2021, 3:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2021, 3:15 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lntue added inline comments.Nov 15 2021, 4:47 PM
clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
59

Look like diag() is used to print messages in this module?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
13

Should be <cstdio> and without newline separation form rest of headers.

39

Why not std::array of appropriate LLVM container?

sivachandra added inline comments.Nov 15 2021, 10:29 PM
clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
1
13

Looks like this is present only for the debug printf?

39

May be static const std::uordered_set<llvm::StringRef>? It would likely make the lookup below much neater.

59

This looks like a debug printf?

michaelrj marked 6 inline comments as done.

clean up the code and remove debug statements

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
13

yes, I didn't mean to leave that in there.

39

an unordered set is a good idea, but the documentation for StringRef says it's best not to use them for storage, so I went with std::string instead. The code is still a lot nicer.

59

yes, I didn't mean to leave that in there.

add test for the new lint behavior:

michaelrj marked an inline comment as done.Nov 16 2021, 11:22 AM
lntue accepted this revision.Nov 16 2021, 11:28 AM
This revision is now accepted and ready to land.Nov 16 2021, 11:28 AM
sivachandra accepted this revision.Nov 16 2021, 11:38 AM

For libc requirements, LGTM. Please wait for @aaron.ballman for stamping the clang-tidy parts.

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
39

Literal strings will not count as "storage". They are global data. On the other hand, std::string will make copies of the literals and require "storage".

michaelrj updated this revision to Diff 387759.Nov 16 2021, 2:00 PM
michaelrj marked an inline comment as done.

update the set type

michaelrj added inline comments.Nov 17 2021, 10:44 AM
clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
39

When I tried to put a StringRef into an unordered_set it told me that StringRef doesn't have a hash function, so I went looking for what the proper, LLVM way of doing this was. In the end, I discovered that unordered_set is not recommended, and that StringSet does exactly what we need here, so I've switched to that.

ping for clang-tidy review

aaron.ballman added inline comments.Nov 18 2021, 11:21 AM
clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
39

This is more in line with the coding style guidelines.

40

Is there a reason that aligned_alloc function is excluded?

55–57

Style guideline nits.

michaelrj marked 3 inline comments as done.

address comments and rebase

aaron.ballman accepted this revision.Nov 30 2021, 7:22 AM

LGTM aside from a tiny nit with the documentation.

libc/docs/clang_tidy_checks.rst
71

fix final comments before commit

michaelrj marked an inline comment as done.Nov 30 2021, 11:45 AM
This revision was landed with ongoing or failed builds.Nov 30 2021, 11:46 AM
This revision was automatically updated to reflect the committed changes.