[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

20

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.

27

nit: remove the blank line.

34

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

5

nit: remove extra trailing ===

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

14

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

17

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

20

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.

34

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

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

14

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

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

58

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

70

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

14

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

31

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

The same below.

65

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

76

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

29

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 "../ClangTidyModuleRegistry.h"
 #include "CreatUsageCheck.h"
 #include "FileOpenFlagCheck.h"
+#include "FopenModeCheck.h"
 
 using namespace clang::ast_matchers;
 

   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FileOpenFlagCheck>("android-file-open-flag");
     CheckFactories.registerCheck<CreatUsageCheck>("android-creat-usage");
+    CheckFactories.registerCheck<FopenModeCheck>("android-fopen-mode");
   }
 };
 

clang-tidy/android/CMakeLists.txt

   AndroidTidyModule.cpp
   CreatUsageCheck.cpp
   FileOpenFlagCheck.cpp
+  FopenModeCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tidy/android/FopenModeCheck.h

+//===--- FopenModeCheck.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_FOPEN_MODE_STRING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_FOPEN_MODE_STRING_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-fopen-mode.html
+class FopenModeCheck : public ClangTidyCheck {
+public:
+  FopenModeCheck(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_FOPEN_MODE_STRING_H

clang-tidy/android/FopenModeCheck.cpp

+//===--- FopenModeCheck.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 "FopenModeCheck.h"
+#include "../utils/ExprToStr.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 utils::ExprToStr(Arg, SM, LangOpts) + " \"" + MODE + "\"";
+
+  StringRef SR = cast<StringLiteral>(Arg->IgnoreParenCasts())->getString();
+  return "\"" + SR.str() + MODE + "\"";
+}
+} // namespace
+
+void FopenModeCheck::registerMatchers(MatchFinder *Finder) {
+  auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter())));
+
+  Finder->addMatcher(
+      callExpr(
+          callee(functionDecl(isExternC(), hasParameter(0, CharPointerType),
+                              hasParameter(1, CharPointerType),
+                              hasName("fopen"), returns(asString("FILE *")))
+                     .bind("funcDecl")))
+          .bind("fopenFn"),
+      this);
+}
+
+void FopenModeCheck::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);
+
+  if (!isa<StringLiteral>(ModeArg->IgnoreParenCasts()))
+    return;
+
+  // Check if the 'e' may be in the mode string.
+  StringRef ModeStr =
+      cast<StringLiteral>(ModeArg->IgnoreParenCasts())->getString();
+  if (ModeStr.find(MODE) != StringRef::npos)
+    return;
+
+  const SourceManager &SM = *Result.SourceManager;
+  std::string ReplacementText = BuildReplaceText(ModeArg, SM, getLangOpts());
+
+  SourceRange ModeRange(ModeArg->getLocStart(), ModeArg->getLocEnd());
+
+  diag(ModeRange.getBegin(), "use %0() mode 'e' to set O_CLOEXEC.")
+      << FD->getName()
+      << FixItHint::CreateReplacement(ModeRange, 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-fopen-mode
+  <http://clang.llvm.org/extra/clang-tidy/checks/android-fopen-mode>`_ 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-fopen-mode.rst

+.. title:: clang-tidy - android-fopen-mode
+
+android-fopen-mode
+==================
+
+``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-fopen-mode.cpp

+// RUN: %check_clang_tidy %s android-fopen-mode %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-fopen-mode]
+  // CHECK-FIXES: "re"
+
+  fopen("filename", FILE_OPEN_RO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use fopen() mode 'e'
+  // CHECK-FIXES: 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:
+  char arr[2] = "r";
+  fopen("filename", arr);
+  // 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:
+  }
+};