Page MenuHomePhabricator

[clang-tidy] Add check callee-namespace.
ClosedPublic

Authored by PaulkaToast on Apr 26 2020, 7:21 PM.

Details

Summary

This check will ensure that all calls to functions resolve to one inside the __llvm_libc namespace.

This is done to ensure that if we include a public header then we don't accidentally call into the a function within the global namespace.

Diff Detail

Event Timeline

PaulkaToast created this revision.Apr 26 2020, 7:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2020, 7:21 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
119

Please enclose __llvm_libc in double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-callee-namespace.rst
7

Please enclose __llvm_libc in double back-ticks.

PaulkaToast marked 2 inline comments as done.
aaron.ballman added inline comments.Apr 27 2020, 2:27 PM
clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
23–24

Under what circumstances could the translation unit be passed in as the declaration? I think this code can just be removed.

31–32

Do you also want to catch binding of function pointers? e.g.,

float (*fp)(float) = sinf;
44–47
const auto *NS = dyn_cast<NamespaceDecl>(getOutermostNamespace(FuncDecl));
if (NS && NS->getName() == "__llvm_libc")
  return;
49–50

How about: %0 must resolve to a function declared within the '__llvm_libc' namespace

53–54

This diagnostic seems a bit strange -- I would expect some text after the colon.

PaulkaToast marked 8 inline comments as done.
PaulkaToast added inline comments.
clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
23–24

Actually you are totally right. This case would never occur, thanks (:

31–32

I hadn't considered function pointers, does seem like a good thing to catch though. Modified the matches to catch this case as well. Thank you!

53–54

I was trying mimic the clang's previous definition diagnostic. e.g. : https://godbolt.org/z/V4tKr-
Although the colon does seem to confusing so I removed it.

aaron.ballman accepted this revision.Apr 28 2020, 11:04 AM

LGTM aside from some minor nits.

clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
23–25

Maybe const DeclContext *Parent = Decl->getParent(); and then use Parent in this function? Not critical, but it would simplify things a bit.

53–54

Ah, thank you for the explanation. I think I would word it resolves to this declaration (or something along those lines) to be a bit less grammatically ambiguous. When the diagnostic ends in "to", I assume there's a part of the diagnostic missing and I wonder "to what?"

This revision is now accepted and ready to land.Apr 28 2020, 11:04 AM
PaulkaToast marked 3 inline comments as done.
PaulkaToast added inline comments.
clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
53–54

I agree with this, thanks for the insight (:

This revision was automatically updated to reflect the committed changes.