[clang-tidy] Function names configurable for cppcoreguidelines-nomalloc - checker

[JonasToth]

Summary

Hello everybody,

this is an incremental patch for the NoMalloc-Checker I wrote. It allows to configure the memory-management functions, that are checked,
This might be helpful for a code base with custom functions in use, or non-standard functionality, like posix_memalign.

[aaron.ballman]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

70 ↗

(On Diff #82904)

functionnames -> function names

71 ↗

(On Diff #82904)

This seems fragile because the SmallVector does not own the StringRef objects, whose lifetimes are tied to the lifetime of the std::string passed to the function.

Also, why commas instead of semicolons? We have parseStringList() in OptionsUtils.h for doing exactly this sort of thing, only with semicolons instead of commas.

83 ↗

(On Diff #82904)

Please do not use auto as the type is not spelled out in the initialization. (Here and elsewhere.)

84 ↗

(On Diff #82904)

Shouldn't size should be size()? (Here and elsewhere.)

clang-tidy/cppcoreguidelines/NoMallocCheck.h

54 ↗

(On Diff #82904)

functionnames -> function names

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst

9 ↗

(On Diff #82904)

Comma after Furthermore.

37 ↗

(On Diff #82904)

allocationfunctions -> allocation functions (same for deallocationfunctions and reallocationfunctions).

[JonasToth]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

71 ↗

(On Diff #82904)

i did not know about parseStringList, i will use this one.

84 ↗

(On Diff #82904)

yes :/

[JonasToth]

implemented the ez issues.

[JonasToth]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

71 ↗

(On Diff #82904)

i adjusted the code to use parseStringList, but the matchers want SmallVector and not std::vector, so there is a ugly copy. is there a way around it?

[JonasToth]

fixed issues pointed out by aaron

[aaron.ballman]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

71 ↗

(On Diff #82904)

Hmm, it seems like the VariadicFunction internal helper would be more useful if it accepted anything that had a begin() and end() function on it. @sbenza, do you think that would be feasible?

That may be more work than it is worth in this patch. If so, I'd say keep the copy but put in a FIXME.

[sbenza]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

71 ↗

(On Diff #82904)

VariadicFunction takes an ArrayRef<T>
ArrayRef is implicitly convertible from vector.
I think the problem is not that it wants a SmallVector.
The problem is that it wants a container of StringRef.

The easiest way do convert is:

auto VectorOfString = parseStringList(List);
... hasAnyName(std::vector<StringRef>(VectorOfString.begin(), VectorOfString.end()));

[JonasToth]

Adjusted the ugly string parsing part.

[JonasToth]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

71 ↗

(On Diff #82904)

I adjusted the code to your fix. I think it looks quite ok so.
Thank you :)

[JonasToth]

ping

[alexfh]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

70–86 ↗

(On Diff #84171)

This should be a single standalone function with a StringRef parameter rather than three class methods.

[JonasToth]

refactored like requested.

[alexfh]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

27 ↗

(On Diff #85594)

functionMatcher says nothing relevant to me, hasNameFromList maybe?

clang-tidy/cppcoreguidelines/NoMallocCheck.h

47–48 ↗

(On Diff #85594)

Is the comment complete? I only got that each variable stores a comma-separated list of function names, after which my parser failed irrecoverably.

[JonasToth]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

27 ↗

(On Diff #85594)

I dont think has.... is a good name, since it kinda suggests a boolean return value.

On the other hand it does semantically the same as hasAnyName, therfore hasNameFromList would work well.

i think functionDecl(fromList(AllocList)) would read quite well.

clang-tidy/cppcoreguidelines/NoMallocCheck.h

47–48 ↗

(On Diff #85594)

Copied from the code:

/// Each variable is a list of comma-seperated function names (with namespace)
/// to memory management functions.

[alexfh]

clang-tidy/cppcoreguidelines/NoMallocCheck.h

47–48 ↗

(On Diff #85594)

On another reading I probably see what you mean. How about

/// Semicolon-separated list of fully qualified names of memory allocation functions the check should warn about. Defaults to `::malloc;::calloc`.
... AllocList;
/// Semicolon-separated list of fully qualified names of memory reallocation functions. Defaults to `::realloc`.
... ReallocList;
/// Semicolon-separated list of fully qualified names of memory deallocation functions. Defaults to `::free`.
.. DeallocList;

? (modulo clang-format)

[alexfh]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

27 ↗

(On Diff #85594)

I still think something derived from hasAnyOfTheNamesSpecifiedInThisSemicolonSeparatedList would be clearer, but I'm fine with fromList as long as the function stays private to this file. If anyone suggests to move this to common utilities we'd need to figure out a less ambiguous name for it.

[JonasToth]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

27 ↗

(On Diff #85594)

i went with hasAnyListedName, i hope thats ok.

[JonasToth]

renamed function as requested.

[JonasToth]

oversaw the docs, but fixed now as well

[JonasToth]

updated doc strings

[aaron.ballman]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

28 ↗

(On Diff #87094)

Please don't use auto here -- the type is not immediately apparent from the initialization.

29 ↗

(On Diff #87094)

I don't think this should be an assert -- that means we crash clang-tidy if the user enters an empty list for some reason. Perhaps a more user-friendly approach would be to diagnose that situation but then continue processing without matching anything?

Also, this should be !NameList.empty().

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst

9 ↗

(On Diff #87094)

"other/more" reads rather awkwardly. How about "...it can be configured to check against a user-specified list of functions..." instead?

32 ↗

(On Diff #87094)

semicolon-separated (with the hyphen).

[JonasToth]

Adjust to issues aaron pointed out.

• allow empty function lists
• doc issues resolved
• removed auto

[JonasToth]

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

28 ↗

(On Diff #87094)

i do it, but it reduces readability IMHO.

29 ↗

(On Diff #87094)

removed it, added a check for empty lists as well, to not crash clang-tidy

[aaron.ballman]

There are some minor comment typos, but otherwise LGTM. Please wait for @alexfh to sign off as well though, since he had requested some changes.

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst

9 ↗

(On Diff #87094)

A few typos:

user-specific -> user-specified
lisf -> list.

[JonasToth]

fix two typos pointed out by aaron

[JonasToth]

alrighty :)
i dont have commit rights though.
please some commit when accepted finally. :)

[alexfh]

LG with a couple of nits. I'll fix and commit for you.

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst

33 ↗

(On Diff #87163)

nit: "use case" are two words. But I'd better remove this paragraph altogether and expand the option descriptions as per the comment below.

38 ↗

(On Diff #87163)

Semicolon-separated list of fully qualified names of memory allocation functions. Should contain at least one function name. Defaults to .... Same for the other two. And the clarification above becomes redundant.

[alexfh]

Missed one more thing: please mention the check in the clang-tools-extra release notes (and since you're doing that, also address the comments).

[JonasToth]

I worked on it, sry for the long delay.
Release Note adjusted, checker doc adjusted as well. As it turns out, it is possible
to have empty lists for the function lists, so i removed this requirement in the doc.

I found a bug in another part of the doc, where the link to the cppguidelines is broken.
see http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html

Since iam not confident in rst format, is this change usefull? I saw this in the release notes, (the _ character after the link).

• Merge branch 'master' of http://llvm.org/git/clang-tools-extra into feature_config_malloc
• [Doc] adjust documentation like requested in review

[JonasToth]

mark as done

[alexfh]

docs/ReleaseNotes.rst

66 ↗

(On Diff #89809)

The format of links is text <http://url/>_ (so your text is missing <> around the url).

You can verify the result by installing Sphinx and building the docs-clang-tools-html target.

[JonasToth]

fix the link in release notes

[alexfh]

Thanks! LG

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NoMallocCheck.h

 /// alternatives to it.
 /// The check is only enabled in C++. For analyzing malloc calls see Clang
 /// Static Analyzer - unix.Malloc.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html
 class NoMallocCheck : public ClangTidyCheck {
 public:
+  /// Construct Checker and read in configuration for function names.
   NoMallocCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : ClangTidyCheck(Name, Context),
+        AllocList(Options.get("Allocations", "::malloc;::calloc")),
+        ReallocList(Options.get("Reallocations", "::realloc")),
+        DeallocList(Options.get("Deallocations", "::free")) {}
+
+  /// Make configuration of checker discoverable.
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
   /// Registering for malloc, calloc, realloc and free calls.
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
 
   /// Checks matched function calls and gives suggestion to modernize the code.
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  /// Semicolon-seperated list of fully qualified names of memory allocation
+  /// functions the check warns about. Defaults to `::malloc;::calloc`.
+  const std::string AllocList;
+  /// Semicolon-seperated list of fully qualified names of memory reallocation 
+  /// functions the check warns about. Defaults to `::realloc`.
+  const std::string ReallocList;
+  /// Semicolon-seperated list of fully qualified names of memory deallocation 
+  /// functions the check warns about. Defaults to `::free`.
+  const std::string DeallocList;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_NO_MALLOC_H

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NoMallocCheck.cpp

 //===--- NoMallocCheck.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 "NoMallocCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include <iostream>
+#include <algorithm>
 #include <string>
+#include <vector>
 
 using namespace clang::ast_matchers;
+using namespace clang::ast_matchers::internal;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
+namespace {
+Matcher<FunctionDecl> hasAnyListedName(const std::string &FunctionNames) {
+  const std::vector<std::string> NameList =
+      utils::options::parseStringList(FunctionNames);
+  return hasAnyName(std::vector<StringRef>(NameList.begin(), NameList.end()));
+}
+}
+
+void NoMallocCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Allocations", AllocList);
+  Options.store(Opts, "Reallocations", ReallocList);
+  Options.store(Opts, "Deallocations", DeallocList);
+}
+
 void NoMallocCheck::registerMatchers(MatchFinder *Finder) {
   // C-style memory management is only problematic in C++.
   if (!getLangOpts().CPlusPlus)
     return;
 
   // Registering malloc, will suggest RAII.
-  Finder->addMatcher(
-      callExpr(callee(functionDecl(hasAnyName("::malloc", "::calloc"))))
-          .bind("aquisition"),
+  Finder->addMatcher(callExpr(callee(functionDecl(hasAnyListedName(AllocList))))
+                         .bind("allocation"),
                      this);
 
   // Registering realloc calls, suggest std::vector or std::string.
   Finder->addMatcher(
-      callExpr(callee(functionDecl(hasName("::realloc")))).bind("realloc"),
+      callExpr(callee(functionDecl(hasAnyListedName(ReallocList))))
+          .bind("realloc"),
       this);
 
   // Registering free calls, will suggest RAII instead.
   Finder->addMatcher(
-      callExpr(callee(functionDecl(hasName("::free")))).bind("free"), this);
+      callExpr(callee(functionDecl(hasAnyListedName(DeallocList))))
+          .bind("free"),
+      this);
 }
 
 void NoMallocCheck::check(const MatchFinder::MatchResult &Result) {
   const CallExpr *Call = nullptr;
   StringRef Recommendation;
 
-  if ((Call = Result.Nodes.getNodeAs<CallExpr>("aquisition")))
+  if ((Call = Result.Nodes.getNodeAs<CallExpr>("allocation")))
     Recommendation = "consider a container or a smart pointer";
   else if ((Call = Result.Nodes.getNodeAs<CallExpr>("realloc")))
     Recommendation = "consider std::vector or std::string";
   else if ((Call = Result.Nodes.getNodeAs<CallExpr>("free")))
     Recommendation = "use RAII";
 
   assert(Call && "Unhandled binding in the Matcher");
 
   diag(Call->getLocStart(), "do not manage memory manually; %0")
       << Recommendation << SourceRange(Call->getLocStart(), Call->getLocEnd());
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 Improvements to clang-tidy
 --------------------------
 
 - New `cert-dcl58-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl58-cpp.html>`_ check
 
   Finds modification of the ``std`` or ``posix`` namespace.
 
+- Improved `cppcoreguidelines-no-malloc 
+  <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html>`_ check
+
+  Allow custom memory management functions to be considered as well.
+
 - New `readability-misleading-indentation
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html>`_ check
 
   Finds misleading indentation where braces should be introduced or the code should be reformatted.
 
 - Added `ParameterThreshold` to `readability-function-size`.
 
   Finds functions that have more then `ParameterThreshold` parameters and emits a warning.

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst

 .. title:: clang-tidy - cppcoreguidelines-no-malloc
 
 cppcoreguidelines-no-malloc
 ===========================
 
 This check handles C-Style memory management using ``malloc()``, ``realloc()``,
 ``calloc()`` and ``free()``. It warns about its use and tries to suggest the use
 of an appropriate RAII object.
-See `C++ Core Guidelines
-<https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rr-mallocfree>`.
+Furthermore, it can be configured to check against a user-specified list of functions 
+that are used for memory management (e.g. ``posix_memalign()``).
+See `C++ Core Guidelines <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rr-mallocfree>`_.
 
 There is no attempt made to provide fix-it hints, since manual resource
 management isn't easily transformed automatically into RAII.
 
 .. code-block:: c++
 
   // Warns each of the following lines.
   // Containers like std::vector or std::string should be used.
   char* some_string = (char*) malloc(sizeof(char) * 20);
   char* some_string = (char*) realloc(sizeof(char) * 30);
   free(some_string);
 
   int* int_array = (int*) calloc(30, sizeof(int));
 
   // Rather use a smartpointer or stack variable.
   struct some_struct* s = (struct some_struct*) malloc(sizeof(struct some_struct));
 
+Options
+-------
+
+.. option:: Allocations
+
+   Semicolon-separated list of fully qualified names of memory allocation functions. 
+   Defaults to ``::malloc;::calloc``.
+
+.. option:: Deallocations
+
+   Semicolon-separated list of fully qualified names of memory allocation functions. 
+   Defaults to ``::free``.
+
+.. option:: Reallocations
+
+   Semicolon-separated list of fully qualified names of memory allocation functions. 
+   Defaults to ``::realloc``.
+

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-no-malloc-custom.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-no-malloc %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-no-malloc.Allocations, value: "::malloc;::align_malloc;::calloc"},\
+// RUN:   {key: cppcoreguidelines-no-malloc.Reallocations, value: "::realloc;::align_realloc"},\
+// RUN:   {key: cppcoreguidelines-no-malloc.Deallocations, value: "::free;::align_free"}]}' \
+// RUN: --
+
+using size_t = __SIZE_TYPE__;
+
+void *malloc(size_t size);
+void *align_malloc(size_t size, unsigned short aligmnent);
+void *calloc(size_t num, size_t size);
+void *realloc(void *ptr, size_t size);
+void *align_realloc(void *ptr, size_t size, unsigned short alignment);
+void free(void *ptr);
+void *align_free(void *ptr);
+
+void malloced_array() {
+  int *array0 = (int *)malloc(sizeof(int) * 20);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not manage memory manually; consider a container or a smart pointer [cppcoreguidelines-no-malloc]
+
+  int *zeroed = (int *)calloc(20, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not manage memory manually; consider a container or a smart pointer [cppcoreguidelines-no-malloc]
+
+  int *aligned = (int *)align_malloc(20 * sizeof(int), 16);
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: do not manage memory manually; consider a container or a smart pointer [cppcoreguidelines-no-malloc]
+
+  // reallocation memory, std::vector shall be used
+  char *realloced = (char *)realloc(array0, 50 * sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: do not manage memory manually; consider std::vector or std::string [cppcoreguidelines-no-malloc]
+
+  char *align_realloced = (char *)align_realloc(aligned, 50 * sizeof(int), 16);
+  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: do not manage memory manually; consider std::vector or std::string [cppcoreguidelines-no-malloc]
+
+  // freeing memory the bad way
+  free(realloced);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not manage memory manually; use RAII [cppcoreguidelines-no-malloc]
+
+  align_free(align_realloced);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not manage memory manually; use RAII [cppcoreguidelines-no-malloc]
+  
+  // check if a call to malloc as function argument is found as well
+  free(malloc(20));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not manage memory manually; use RAII [cppcoreguidelines-no-malloc]
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: do not manage memory manually; consider a container or a smart pointer [cppcoreguidelines-no-malloc]
+}
+
+/// newing an array is still not good, but not relevant to this checker
+void newed_array() {
+  int *new_array = new int[10]; // OK(1)
+}
+
+void arbitrary_call() {
+  // we dont want every function to raise the warning even if malloc is in the name
+  malloced_array(); // OK(2)
+
+  // completly unrelated function call to malloc
+  newed_array(); // OK(3)
+}

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-no-malloc-no-functions.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-no-malloc %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-no-malloc.Allocations, value: "::malloc"},\
+// RUN:   {key: cppcoreguidelines-no-malloc.Reallocations, value: ""},\
+// RUN:   {key: cppcoreguidelines-no-malloc.Deallocations, value: ""}]}' \
+// RUN: --
+
+// Just ensure, the check will not crash, when no functions shall be checked.
+
+using size_t = __SIZE_TYPE__;
+
+void *malloc(size_t size);
+
+void malloced_array() {
+  int *array0 = (int *)malloc(sizeof(int) * 20);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not manage memory manually; consider a container or a smart pointer [cppcoreguidelines-no-malloc]
+}