[clang-tidy][Part3] Add a new module Android and three new checks.

[yawanng]

Summary

• fopen() should include "e" in their mode string. [android-fopen-mode]

[yawanng]

Modify the format.

[hokein]

clang-tidy/android/FopenModeCheck.h

19 ↗

(On Diff #101129)

Is there a guideline/code style describing it? If so, would be nice to add the link.

And I'd suggest you to put the detailed document to the rst instead of code comments -- because users usually read documents.

26 ↗

(On Diff #101129)

nit: remove the blank line.

33 ↗

(On Diff #101129)

No need to put utility functions/variables inside the class, you can define them in an anonymous namespace in the implementation file, the same below.

docs/clang-tidy/checks/android-fopen-mode.rst

4 ↗

(On Diff #101129)

nit: remove extra trailing ===

test/clang-tidy/android-fopen-mode.cpp

13 ↗

(On Diff #101129)

Use completed statements fopen("filename", "re"); to do the fix verification. The same below.

16 ↗

(On Diff #101129)

You can use a shorter string here -- It is sufficient as we have a completed message check in the first CHECK-MESSAGES.

[yawanng]

clang-tidy/android/FopenModeCheck.h

19 ↗

(On Diff #101129)

Adding 'e' is like adding the "O_CLOEXEC". The reason is the same with part1(https://reviews.llvm.org/D33304). Added more text in doc.

33 ↗

(On Diff #101129)

Inline one and move the other and the variable to anonymous namespace.

test/clang-tidy/android-fopen-mode.cpp

13 ↗

(On Diff #101129)

The fix will mark the corresponding argument and suggest the fix like:

fopen("filename", "r");

^~
 re

Is this clear to show the fix?

[hokein]

clang-tidy/android/FopenModeCheck.cpp

31 ↗

(On Diff #101270)

We may not need this utility function, see my comment on your part 2.

57 ↗

(On Diff #101251)

How about if (const auto* ModeStr = dyn_cast<StringLiteral>(...))?

69 ↗

(On Diff #101251)

remove the trailing ., clang-tidy message is not a sentence.

And we can simplify the code like: diag(...) << FD << FixItHint::CreateReplacement(ModeRange.getSourceRange(), ReplacementText);

test/clang-tidy/android-fopen-mode.cpp

13 ↗

(On Diff #101129)

CHECK-FIXES is not used to check the message of the fix. It is used to check the code after the generated-fix is applied. So here should be // CHECK-FIXES: fopen("filename", "re");.

[yawanng]

url format fix in doc.rst

[hokein]

clang-tidy/android/FopenModeCheck.cpp

30 ↗

(On Diff #103795)

You can use Twine here which is more efficient: (getSourceText(...) + " \"" + Twine(Mode) + "\"").str().

The same below.

64 ↗

(On Diff #101830)

We'd prefer early return in llvm, and with that the flag "MayHaveE" can be removed.

75 ↗

(On Diff #101830)

I think you can use ModeArg->getSourceRange().

Moreover, as ModeRange is used only once, you can ModeArg->getSourceRange() directly in FixtItHint below.

[hokein]

One comment on test, otherwise looks good.

test/clang-tidy/android-fopen-mode.cpp

28 ↗

(On Diff #104004)

What if str = "re" here? Seems to me the check will omit this case. Would be nice to add this test case.

[yawanng]

rename the check.

[chh]

Summary should be updated. s/android-fopen-mode/android-cloexec-fopen/

clang-tidy/android/AndroidTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "CloexecCreatCheck.h"
+#include "CloexecFopenCheck.h"
 #include "FileOpenFlagCheck.h"
 
 using namespace clang::ast_matchers;

   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FileOpenFlagCheck>("android-file-open-flag");
     CheckFactories.registerCheck<CloexecCreatCheck>("android-cloexec-creat");
+    CheckFactories.registerCheck<CloexecFopenCheck>("android-cloexec-fopen");
   }
 };
 

clang-tidy/android/CMakeLists.txt

 add_clang_library(clangTidyAndroidModule
   AndroidTidyModule.cpp
   CloexecCreatCheck.cpp
+  CloexecFopenCheck.cpp
   FileOpenFlagCheck.cpp
 
   LINK_LIBS

clang-tidy/android/CloexecFopenCheck.h

+//===--- CloexecFopenCheck.h - clang-tidy------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source //
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_FOPEN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_FOPEN_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+/// fopen() is suggested to include "e" in their mode string; like "re" would be
+/// better than "r".
+///
+/// This check only works when corresponding argument is StringLiteral. No
+/// constant propagation.
+///
+/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-fopen.html
+class CloexecFopenCheck : public ClangTidyCheck {
+public:
+  CloexecFopenCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace android
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_FOPEN_H

clang-tidy/android/CloexecFopenCheck.cpp

+//===--- CloexecFopenCheck.cpp - clang-tidy--------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.  //
+//===----------------------------------------------------------------------===//
+
+#include "CloexecFopenCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+namespace {
+static const char MODE = 'e';
+
+// Build the replace text. If it's string constant, add 'e' directly in the end
+// of the string. Else, add "e".
+std::string BuildReplaceText(const Expr *Arg, const SourceManager &SM,
+                             const LangOptions &LangOpts) {
+  if (Arg->getLocStart().isMacroID())
+    return (Lexer::getSourceText(
+                CharSourceRange::getTokenRange(Arg->getSourceRange()), SM,
+                LangOpts) +
+            " \"" + Twine(MODE) + "\"")
+        .str();
+
+  StringRef SR = cast<StringLiteral>(Arg->IgnoreParenCasts())->getString();
+  return ("\"" + SR + Twine(MODE) + "\"").str();
+}
+} // namespace
+
+void CloexecFopenCheck::registerMatchers(MatchFinder *Finder) {
+  auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter())));
+
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(isExternC(), returns(asString("FILE *")),
+                                   hasName("fopen"),
+                                   hasParameter(0, CharPointerType),
+                                   hasParameter(1, CharPointerType))
+                          .bind("funcDecl")))
+          .bind("fopenFn"),
+      this);
+}
+
+void CloexecFopenCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCall = Result.Nodes.getNodeAs<CallExpr>("fopenFn");
+  const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("funcDecl");
+  const Expr *ModeArg = MatchedCall->getArg(1);
+
+  // Check if the 'e' may be in the mode string.
+  const auto *ModeStr = dyn_cast<StringLiteral>(ModeArg->IgnoreParenCasts());
+  if (!ModeStr || (ModeStr->getString().find(MODE) != StringRef::npos))
+    return;
+
+  const std::string &ReplacementText = BuildReplaceText(
+      ModeArg, *Result.SourceManager, Result.Context->getLangOpts());
+
+  diag(ModeArg->getLocStart(), "use %0 mode 'e' to set O_CLOEXEC")
+      << FD
+      << FixItHint::CreateReplacement(ModeArg->getSourceRange(),
+                                      ReplacementText);
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

   Checks if the required file flag ``O_CLOEXEC`` exists in ``open()``,
   ``open64()`` and ``openat()``.
 
+- New `android-cloexec-fopen
+  <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-fopen.html>`_ check
+
+  Checks if the required mode ``e`` exists in the mode argument of ``fopen()``.
+
 - New `cert-dcl21-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl21-cpp.html>`_ check
 

docs/clang-tidy/checks/android-cloexec-fopen.rst

+.. title:: clang-tidy - android-cloexec-fopen
+
+android-cloexec-fopen
+=====================
+
+``fopen()`` should include ``e`` in their mode string; so ``re`` would be
+valid. This is equivalent to having set ``FD_CLOEXEC on`` that descriptor.
+
+Examples:
+
+.. code-block:: c++
+
+  fopen("fn", "r");
+
+  // becomes
+
+  fopen("fn", "re");
+

test/clang-tidy/android-cloexec-fopen.cpp

+// RUN: %check_clang_tidy %s android-cloexec-fopen %t
+
+#define FILE_OPEN_RO "r"
+
+typedef int FILE;
+
+extern "C" FILE *fopen(const char *filename, const char *mode, ...);
+extern "C" FILE *open(const char *filename, const char *mode, ...);
+
+void f() {
+  fopen("filename", "r");
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'fopen' mode 'e' to set O_CLOEXEC [android-cloexec-fopen]
+  // CHECK-FIXES: fopen("filename", "re");
+
+  fopen("filename", FILE_OPEN_RO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'fopen' mode 'e'
+  // CHECK-FIXES: fopen("filename", FILE_OPEN_RO "e");
+
+  fopen("filename", "er");
+  // CHECK-MESSAGES-NOT: warning:
+  fopen("filename", "re");
+  // CHECK-MESSAGES-NOT: warning:
+  fopen("filename", "e");
+  // CHECK-MESSAGES-NOT: warning:
+  open("filename", "e");
+  // CHECK-MESSAGES-NOT: warning:
+
+  char *str = "r";
+  fopen("filename", str);
+  // CHECK-MESSAGES-NOT: warning:
+  str = "re";
+  fopen("filename", str);
+  // CHECK-MESSAGES-NOT: warning:
+  char arr[2] = "r";
+  fopen("filename", arr);
+  // CHECK-MESSAGES-NOT: warning:
+  char arr2[3] = "re";
+  fopen("filename", arr2);
+  // CHECK-MESSAGES-NOT: warning:
+}
+
+namespace i {
+int *fopen(const char *filename, const char *mode, ...);
+void g() {
+  fopen("filename", "e");
+  // CHECK-MESSAGES-NOT: warning:
+}
+} // namespace i
+
+class C {
+public:
+  int *fopen(const char *filename, const char *mode, ...);
+  void h() {
+    fopen("filename", "e");
+    // CHECK-MESSAGES-NOT: warning:
+  }
+};