[clang-tidy] introduce legacy resource functions to 'cppcoreguidelines-owning-memory'

[JonasToth]

Summary

This patch introduces support for legacy C-style resource functions that must obey
the 'owner<>' semantics.

• added legacy creators like malloc,fopen,...
• added legacy consumers like free,fclose,...

This helps codes that mostly benefit from owner:
Legacy, C-Style code that isn't feasable to port directly to RAII but needs a step in between
to identify actual resource management and just using the resources.

[JonasToth]

• Clean up stuff that slipped through

[aaron.ballman]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp

53 ↗

(On Diff #117120)

can not -> cannot

62 ↗

(On Diff #117120)

can not -> cannot

86 ↗

(On Diff #117120)

Remove the backticks from the comment. Also, this seems like a pretty major false positive -- any way to suppress it?

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

Missing ::aligned_alloc

There's a comment below about C-style functions that create resources -- do you care about things from thread.h, mutex.h, or other resource-creating C functions as well?

docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst

89 ↗

(On Diff #117120)

can not -> cannot

95 ↗

(On Diff #117120)

can not -> cannot

test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp

17 ↗

(On Diff #117120)

Missing a test for calloc() and aligned_alloc().

134 ↗

(On Diff #117120)

can not -> cannot

[JonasToth]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp

86 ↗

(On Diff #117120)

The comment is misleading!
The tests actually do have this case and it is caught correctly. (see line 126-130 in testfile).

The false positive that does exist is std::free(std::malloc(100)), because the argument to free is a non-owner beeing initialized with a newly created resource.

I adjust the comment.

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

In general, i care about everything beeing a resource! I just didn't think long enough to come up with everything from the standard (and i don't know enough as well :D)

So this list should contain everything and if you point to me to the right corners i will search the references.

The docs could also mention other common functions (posix, Windows, maybe even CUDA and the like). But i am not so sure about that.

[JonasToth]

• fix typos and wrong comment about false positive

[JonasToth]

test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp

17 ↗

(On Diff #117120)

Would it be enough, to check if
ìnt* Test = calloc(100) works?
Since the matcher either works for everything or nothing, that would kind of work.

Or do you require more testing code?

[aaron.ballman]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

50 ↗

(On Diff #117158)

Comment is out of date.

29 ↗

(On Diff #117120)

We usually only add the APIs that are most common to users of the check, but when in doubt we cover just the standards-based functions, since everyone (generally) has access to those.

I think aligned_alloc() was the only one that's missing from C, but that's going from my (faulty) memory.

docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst

90 ↗

(On Diff #117158)

This comment is now out of date.

test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp

17 ↗

(On Diff #117120)

It's good to test all of the functions when possible to easily do it so that we don't accidentally introduce regressions with just one of them.

[JonasToth]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

Should i ask the guideline people about facilities from C that shall be treated as resources?

IMHO everything from the C/C++ Standards that is a resource/has resource semantics and is not RAII should be catched here.

Adding more OS stuff, that might be a resource (sockets, dunno what else) could be good as well.
Maybe suggestions for these things in the docs would be nice. But i actually don't know enough about these issues to create them myself. But that is optional and maybe the guidelines will even add guidence on this.

[aaron.ballman]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

Should i ask the guideline people about facilities from C that shall be treated as resources?

I think that's a good idea. There are mutex, thread, thread storage creation functions, etc -- should those be treated the same as malloc() and friends?

Adding more OS stuff, that might be a resource (sockets, dunno what else) could be good as well.

Maybe suggestions for these things in the docs would be nice. But i actually don't know enough about these issues to create them myself. But that is optional and maybe the guidelines will even add guidence on this.

This is much harder because it's pretty open-ended. I would say it's fine to leave OS and POSIX functionality out for right now.

[JonasToth]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

I opened an issue here https://github.com/isocpp/CppCoreGuidelines/issues/1050
Lets see what comes out of it. :)

I will add a comment in the docs that functionality from the operating system can qualify as resources, but that everybody needs to modify then configuration to fit his/her needs.

[JonasToth]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

The Coreguideline guys understood it as workoffloading :(, so the list iam currently thinking of is:

• malloc,calloc,realloc,free, aligned_alloc,, these are the obvious one.
• strdup,strndupand maybe the getline class of functions, these are not standardized, but a ts if i understood correctly. http://en.cppreference.com/w/c/experimental/dynamic

But getline does not return a value, it expects a char** where the allocation will happen, so its not very nice for the check.

• fopen,fopen_s,freopen,freopen_s,fclose,tmpfile,tmpfile_s for file IO, all _s functions are again not nice with the check.

For the threading functions:
They all simulate classes with RAII, so in a sense they could be considered as owners, but the whole owner<> thing does not seem to play nice with these utilities. Since the check requires C++11 to have a templated typedef and the C++ standard has all of the funcitonality this check could ignore these functions. For completeness:

• cnd_init, cnd_destroy, tss_create, tss_delete, mtx_init, mtx_destroy

My proposed (for now) list would be the classical memory allocations, strdup and the classical fileio-functions.

[aaron.ballman]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

Rather than an incomplete list of POSIX functions like strdup(), I'd say the first round of the patch would make sense to hit all the C11 standard functions (you can elect to support Annex K or not as you see fit) and then a follow-up patch for getting a mostly comprehensive list of POSIX functions if that's desirable. Otherwise, your list looks good.

[JonasToth]

work on the list of considered functions for legacy resource functions.

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

Posix and nonfitting functions could be done later. But i honestly don't want to put a lot of effort into C11 stuff and more about actual legacy code that.

IMHO: if people write C11 in C++, they don't want tooling support and the gsl.

[JonasToth]

• list of legacy functions updated, test come later

[JonasToth]

• add unittesting both for legacy function and demonstrate bad result with std::vector

I added basic testing for legacy functions. Please request more if necessary, but for now
i think the tests actually cover all possible cases, since the legacy functions either
all work on none of them work. That's why did not add all possible usecases for every single function.

There is a false positive for freopen, since it takes 3 pointer arguments but only one must be an owner<>. The check heuristically assumes that all pointer arguments must be an owner<>, since I don't know a way to encode the position information into the list. This false positive can be fixed with a special cases for freopen, but not in general for now.

Additionally I added a testcase that demonstrates how 'owner<>' could work with
'std::vector<>' as the container to manage resources. Unfortunately this is not
possible right now. Typededuction in templates removes 'owner<>' and uses the underlying
type. The underlying problem is already present in the default testcases.
This additional test could be the base for further work to circumvent the limitation
and tries to make it more obvious.

[JonasToth]

• update doc to explicitly mention, that std::vector<owner<>> does not work properly

[aaron.ballman]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp

82 ↗

(On Diff #118889)

since -> because

227 ↗

(On Diff #118889)

s/seperat, since/separately because

228 ↗

(On Diff #118889)

as pointer -> as a pointer

be a owner -> be an owner

not certain what you mean by "and is known"?

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

IMHO: if people write C11 in C++, they don't want tooling support and the gsl.

I don't think that's a correct assumption to make, but I'm also fine with the more complex bits being done in a follow-up patch.

docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst

24 ↗

(On Diff #118889)

s/as generated/to produce

86 ↗

(On Diff #118889)

The more I look at this option name, the more I think it should be named LegacyResourceProducers to match with LegacyResourceConsumers. What do you think?

(If you decide to make this change, be sure to update the member variables we use as well.)

95 ↗

(On Diff #118889)

s/expection/expecting

114 ↗

(On Diff #118889)

s/beeing/being

[JonasToth]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp

82 ↗

(On Diff #118889)

i dropped the comma, is it correct?
sorry for my bad english grammar :/

228 ↗

(On Diff #118889)

reformulated. Reopen takes 3 pointer arguments, but only one must be an owner. This property is known, hopefully clearer now.

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

29 ↗

(On Diff #117120)

it was stated wrong and too judging. But if c++11 is available, why use C11 threading?

Non-memory related issues might be suitable for another check as well, so it's probably counterproductive.

docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst

86 ↗

(On Diff #118889)

agreed, changed.

[JonasToth]

• address review comments

[aaron.ballman]

clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp

36 ↗

(On Diff #119437)

Producers (plural) here as well as the documentation.

82 ↗

(On Diff #118889)

Yes, this is correct -- and English grammar is not easy, no need to apologize!

227 ↗

(On Diff #118889)

seperatly -> separately

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h

53 ↗

(On Diff #119437)

Should be Producers (plural) here and elsewhere.

29 ↗

(On Diff #117120)

Seems reasonable enough. Can you re-flow this so that :: and calloc are not on different lines?

[JonasToth]

• addressed review comments

[aaron.ballman]

test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp

3 ↗

(On Diff #119493)

I think the RUN line needs to be updated to Producers (plural).

Does this test pass as-is? (That would be something worth looking into if it does.)

[JonasToth]

• fix test line

[aaron.ballman]

LGTM, thank you!

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

 /// Checks for common use cases for gsl::owner and enforces the unique owner
 /// nature of it whenever possible.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-owning-memory.html
 class OwningMemoryCheck : public ClangTidyCheck {
 public:
   OwningMemoryCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : ClangTidyCheck(Name, Context),
+        LegacyResourceProducers(Options.get(
+            "LegacyResourceProducers", "::malloc;::aligned_alloc;::realloc;"
+                                       "::calloc;::fopen;::freopen;::tmpfile")),
+        LegacyResourceConsumers(Options.get(
+            "LegacyResourceConsumers", "::free;::realloc;::freopen;::fclose")) {
+  }
+
+  /// Make configuration of checker discoverable.
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   bool handleDeletion(const ast_matchers::BoundNodes &Nodes);
+  bool handleLegacyConsumers(const ast_matchers::BoundNodes &Nodes);
   bool handleExpectedOwner(const ast_matchers::BoundNodes &Nodes);
   bool handleAssignmentAndInit(const ast_matchers::BoundNodes &Nodes);
   bool handleAssignmentFromNewOwner(const ast_matchers::BoundNodes &Nodes);
   bool handleReturnValues(const ast_matchers::BoundNodes &Nodes);
   bool handleOwnerMembers(const ast_matchers::BoundNodes &Nodes);
+
+  /// List of old C-style functions that create resources.
+  /// Defaults to
+  /// `::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile`.
+  const std::string LegacyResourceProducers;
+  /// List of old C-style functions that consume or release resources.
+  /// Defaults to `::free;::realloc;::freopen;::fclose`.
+  const std::string LegacyResourceConsumers;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_OWNING_MEMORY_H

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

 
 using namespace clang::ast_matchers;
 using namespace clang::ast_matchers::internal;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
+// FIXME: Copied from 'NoMallocCheck.cpp'. Has to be refactored into 'util' or
+// something like that.
+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()));
+}
+} // namespace
+
+void OwningMemoryCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "LegacyResourceProducers", LegacyResourceProducers);
+  Options.store(Opts, "LegacyResourceConsumers", LegacyResourceConsumers);
+}
+
 /// Match common cases, where the owner semantic is relevant, like function
 /// calls, delete expressions and others.
 void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
     return;
 
   const auto OwnerDecl = typeAliasTemplateDecl(hasName("::gsl::owner"));
   const auto IsOwnerType = hasType(OwnerDecl);
+
+  const auto LegacyCreatorFunctions = hasAnyListedName(LegacyResourceProducers);
+  const auto LegacyConsumerFunctions =
+      hasAnyListedName(LegacyResourceConsumers);
+
+  // Legacy functions that are use for resource management but cannot be
+  // updated to use `gsl::owner<>`, like standard C memory management.
+  const auto CreatesLegacyOwner =
+      callExpr(callee(functionDecl(LegacyCreatorFunctions)));
+  // C-style functions like `::malloc()` sometimes create owners as void*
+  // which is expected to be cast to the correct type in C++. This case
+  // must be catched explicitly.
+  const auto LegacyOwnerCast =
+      castExpr(hasSourceExpression(CreatesLegacyOwner));
+  // Functions that do manual resource management but cannot be updated to use
+  // owner. Best example is `::free()`.
+  const auto LegacyOwnerConsumers = functionDecl(LegacyConsumerFunctions);
+
   const auto CreatesOwner =
-      anyOf(cxxNewExpr(), callExpr(callee(functionDecl(
-                              returns(qualType(hasDeclaration(OwnerDecl)))))));
-  const auto ConsideredOwner = anyOf(IsOwnerType, CreatesOwner);
+      anyOf(cxxNewExpr(),
+            callExpr(callee(
+                functionDecl(returns(qualType(hasDeclaration(OwnerDecl)))))),
+            CreatesLegacyOwner, LegacyOwnerCast);
+
+  const auto ConsideredOwner = eachOf(IsOwnerType, CreatesOwner);
 
   // Find delete expressions that delete non-owners.
   Finder->addMatcher(
       cxxDeleteExpr(
           hasDescendant(
               declRefExpr(unless(ConsideredOwner)).bind("deleted_variable")))
           .bind("delete_expr"),
       this);
 
+  // Ignoring the implicit casts is vital because the legacy owners do not work
+  // with the 'owner<>' annotation and therefore always implicitly cast to the
+  // legacy type (even 'void *').
+  //
+  // Furthermore, legacy owner functions are assumed to use raw pointers for
+  // resources. This check assumes that all pointer arguments of a legacy
+  // functions shall be 'gsl::owner<>'.
+  Finder->addMatcher(
+      callExpr(
+          allOf(callee(LegacyOwnerConsumers),
+                hasAnyArgument(allOf(unless(ignoringImpCasts(ConsideredOwner)),
+                                     hasType(pointerType())))))
+          .bind("legacy_consumer"),
+      this);
+
   // Matching assignment to owners, with the rhs not being an owner nor creating
   // one.
   Finder->addMatcher(binaryOperator(allOf(matchers::isAssignmentOperator(),
                                           hasLHS(IsOwnerType),
                                           hasRHS(unless(ConsideredOwner))))
                          .bind("owner_assignment"),
                      this);
 
Finder->addMatcher(
       this);
 }
 
 void OwningMemoryCheck::check(const MatchFinder::MatchResult &Result) {
   const auto &Nodes = Result.Nodes;
 
   bool CheckExecuted = false;
   CheckExecuted |= handleDeletion(Nodes);
+  CheckExecuted |= handleLegacyConsumers(Nodes);
   CheckExecuted |= handleExpectedOwner(Nodes);
   CheckExecuted |= handleAssignmentAndInit(Nodes);
   CheckExecuted |= handleAssignmentFromNewOwner(Nodes);
   CheckExecuted |= handleReturnValues(Nodes);
   CheckExecuted |= handleOwnerMembers(Nodes);
 
   assert(CheckExecuted &&
          "None of the subroutines executed, logic error in matcher!");
if (DeleteStmt) {
     diag(Decl->getLocStart(), "variable declared here", DiagnosticIDs::Note)
         << Decl->getSourceRange();
 
     return true;
   }
   return false;
 }
 
+bool OwningMemoryCheck::handleLegacyConsumers(const BoundNodes &Nodes) {
+  // Result of matching for legacy consumer-functions like `::free()`.
+  const auto *LegacyConsumer = Nodes.getNodeAs<CallExpr>("legacy_consumer");
+
+  // FIXME: `freopen` should be handled seperately because it takes the filename
+  // as a pointer, which should not be an owner. The argument that is an owner
+  // is known and the false positive coming from the filename can be avoided.
+  if (LegacyConsumer) {
+    diag(LegacyConsumer->getLocStart(),
+         "calling legacy resource function without passing a 'gsl::owner<>'")
+        << LegacyConsumer->getSourceRange();
+    return true;
+  }
+  return false;
+}
+
 bool OwningMemoryCheck::handleExpectedOwner(const BoundNodes &Nodes) {
   // Result of function call matchers.
   const auto *ExpectedOwner = Nodes.getNodeAs<Expr>("expected_owner_argument");
 
   // Expected function argument to be owner.
   if (ExpectedOwner) {
     diag(ExpectedOwner->getLocStart(),
          "expected argument of type 'gsl::owner<>'; got %0")

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst

.. code-block:: c++
   namespace gsl { template <typename T> owner = T; }
 
 It is therefore simple to introduce the owner even without using an implementation of
 the `Guideline Support Library <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#gsl-guideline-support-library>`_.
 
 All checks are purely type based and not (yet) flow sensitive.
 
 The following examples will demonstrate the correct and incorrect initializations
-of owners, assignment is handled the same way.
+of owners, assignment is handled the same way. Note that both ``new`` and 
+``malloc()``-like resource functions are considered to produce resources.
 
 .. code-block:: c++
 
   // Creating an owner with factory functions is checked.
   gsl::owner<int*> function_that_returns_owner() { return gsl::owner<int*>(new int(42)); }
 
   // Dynamic memory must be assigned to an owner
   int* Something = new int(42); // BAD, will be caught
.. code-block:: c++
   int NonOwner = 42;
   expects_owner(&NonOwner); // Bad, will get caught
 
   // Good Code
   gsl::owner<int*> Owner = new int(42);
   expects_owner(Owner); // Good
   expects_owner(new int(42)); // Good as well, recognized created resource
 
+  // Port legacy code for better resource-safety
+  gsl::owner<FILE*> File = fopen("my_file.txt", "rw+");
+  FILE* BadFile = fopen("another_file.txt", "w"); // Bad, warned
+
+  // ... use the file
+
+  fclose(File); // Ok, File is annotated as 'owner<>'
+  fclose(BadFile); // BadFile is not an 'owner<>', will be warned
+
+
+Options
+-------
+
+.. option:: LegacyResourceProducers
+
+   Semicolon-separated list of fully qualified names of legacy functions that create
+   resources but cannot introduce ``gsl::owner<>``.
+   Defaults to ``::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile``.
+
+
+.. option:: LegacyResourceConsumers
+
+   Semicolon-separated list of fully qualified names of legacy functions expecting
+   resource owners as pointer arguments but cannot introduce ``gsl::owner<>``.
+   Defaults to ``::free;::realloc;::freopen;::fclose``.
+
+
 Limitations
 -----------
 
 Using ``gsl::owner<T*>`` in a typedef or alias is not handled correctly. 
 
 .. code-block:: c++
 
   using heap_int = gsl::owner<int*>;
   heap_int allocated = new int(42); // False positive!
 
 The ``gsl::owner<T*>`` is declared as a templated type alias.
 In template functions and classes, like in the example below, the information
 of the type aliases gets lost. Therefore using ``gsl::owner<T*>`` in a heavy templated
-code base might lead to false positives.
+code base might lead to false positives. 
+This limitation results in ``std::vector<gsl::owner<T*>>`` not being diagnosed correctly.
 
 .. code-block:: c++
 
   // This template function works as expected. Type information doesn't get lost.
   template <typename T>
   void delete_owner(gsl::owner<T*> owned_object) {
     delete owned_object; // Everything alright
   }

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-owning-memory-containers.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-owning-memory %t
+
+namespace gsl {
+template <typename T>
+using owner = T;
+}
+
+namespace std {
+
+// Not actually a vector, but more a dynamic, fixed size array. Just to demonstrate
+// functionality or the lack of the same.
+template <typename T>
+class vector {
+public:
+  vector(unsigned long size, T val) : data{new T[size]}, size{size} {
+    for (unsigned long i = 0ul; i < size; ++i) {
+      data[i] = val;
+    }
+  }
+
+  T *begin() { return data; }
+  T *end() { return &data[size]; }
+  T &operator[](unsigned long index) { return data[index]; }
+
+private:
+  T *data;
+  unsigned long size;
+};
+
+} // namespace std
+
+// All of the following codesnippets should be valid with appropriate 'owner<>' anaylsis,
+// but currently the type information of 'gsl::owner<>' gets lost in typededuction.
+int main() {
+  std::vector<gsl::owner<int *>> OwnerStdVector(100, nullptr);
+
+  // Rangebased looping in resource vector.
+  for (auto *Element : OwnerStdVector) {
+    Element = new int(42);
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: assigning newly created 'gsl::owner<>' to non-owner 'int *'
+  }
+  for (auto *Element : OwnerStdVector) {
+    delete Element;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead
+    // CHECK-MESSAGES: [[@LINE-3]]:8: note: variable declared here
+  }
+
+  // Indexbased looping in resource vector.
+  for (int i = 0; i < 100; ++i) {
+    OwnerStdVector[i] = new int(42);
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: assigning newly created 'gsl::owner<>' to non-owner 'int *'
+  }
+  for (int i = 0; i < 100; ++i) {
+    delete OwnerStdVector[i];
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead
+    // A note gets emitted here pointing to the return value of the operator[] from the
+    // vector implementation. Maybe this is considered misleading.
+  }
+
+  return 0;
+}

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-owning-memory %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-owning-memory.LegacyResourceProducers, value: "::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile"}, \
+// RUN:   {key: cppcoreguidelines-owning-memory.LegacyResourceConsumers, value: "::free;::realloc;::freopen;::fclose"}]}' \
+// RUN: -- -std=c++11 -nostdlib -nostdinc++
+
+namespace gsl {
+template <class T>
+using owner = T;
+} // namespace gsl
+
+extern "C" {
+using size_t = unsigned long;
+using FILE = int;
+
+void *malloc(size_t ByteCount);
+void *aligned_alloc(size_t Alignment, size_t Size);
+void *calloc(size_t Count, size_t SizeSingle);
+void *realloc(void *Resource, size_t NewByteCount);
+void free(void *Resource);
+
+FILE *tmpfile(void);
+FILE *fopen(const char *filename, const char *mode);
+FILE *freopen(const char *filename, const char *mode, FILE *stream);
+void fclose(FILE *Resource);
+}
+
+namespace std {
+using ::FILE;
+using ::size_t;
+
+using ::fclose;
+using ::fopen;
+using ::freopen;
+using ::tmpfile;
+
+using ::aligned_alloc;
+using ::calloc;
+using ::free;
+using ::malloc;
+using ::realloc;
+} // namespace std
+
+void nonOwningCall(int *Resource, size_t Size) {}
+void nonOwningCall(FILE *Resource) {}
+
+void consumesResource(gsl::owner<int *> Resource, size_t Size) {}
+void consumesResource(gsl::owner<FILE *> Resource) {}
+
+void testNonCasted(void *Resource) {}
+
+void testNonCastedOwner(gsl::owner<void *> Resource) {}
+
+FILE *fileFactory1() { return ::fopen("new_file.txt", "w"); }
+// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'FILE *' (aka 'int *') or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'
+gsl::owner<FILE *> fileFactory2() { return std::fopen("new_file.txt", "w"); } // Ok
+
+int *arrayFactory1() { return (int *)std::malloc(100); }
+// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'int *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'
+gsl::owner<int *> arrayFactory2() { return (int *)std::malloc(100); } // Ok
+void *dataFactory1() { return std::malloc(100); }
+// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'void *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'
+gsl::owner<void *> dataFactory2() { return std::malloc(100); } // Ok
+
+void test_resource_creators() {
+  const unsigned int ByteCount = 25 * sizeof(int);
+  int Bad = 42;
+
+  int *IntArray1 = (int *)std::malloc(ByteCount);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
+  int *IntArray2 = static_cast<int *>(std::malloc(ByteCount)); // Bad
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
+  void *IntArray3 = std::malloc(ByteCount);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>'
+
+  int *IntArray4 = (int *)::malloc(ByteCount);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
+  int *IntArray5 = static_cast<int *>(::malloc(ByteCount)); // Bad
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
+  void *IntArray6 = ::malloc(ByteCount);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>'
+
+  gsl::owner<int *> IntArray7 = (int *)malloc(ByteCount); // Ok
+  gsl::owner<void *> IntArray8 = malloc(ByteCount);       // Ok
+
+  gsl::owner<int *> IntArray9 = &Bad;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected initialization with value of type 'gsl::owner<>'; got 'int *'
+
+  nonOwningCall((int *)malloc(ByteCount), 25);
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: initializing non-owner argument of type 'int *' with a newly created 'gsl::owner<>'
+  nonOwningCall((int *)::malloc(ByteCount), 25);
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: initializing non-owner argument of type 'int *' with a newly created 'gsl::owner<>'
+
+  consumesResource((int *)malloc(ByteCount), 25);   // Ok
+  consumesResource((int *)::malloc(ByteCount), 25); // Ok
+
+  testNonCasted(malloc(ByteCount));
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>'
+  testNonCastedOwner(gsl::owner<void *>(malloc(ByteCount))); // Ok
+  testNonCastedOwner(malloc(ByteCount));                     // Ok
+
+  FILE *File1 = std::fopen("test_name.txt", "w+");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+  FILE *File2 = ::fopen("test_name.txt", "w+");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+
+  gsl::owner<FILE *> File3 = ::fopen("test_name.txt", "w+"); // Ok
+
+  FILE *File4;
+  File4 = ::fopen("test_name.txt", "w+");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: assigning newly created 'gsl::owner<>' to non-owner 'FILE *' (aka 'int *')
+
+  gsl::owner<FILE *> File5;
+  File5 = ::fopen("test_name.txt", "w+"); // Ok
+  File5 = File1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected assignment source to be of type 'gsl::owner<>'; got 'FILE *' (aka 'int *')
+
+  gsl::owner<FILE *> File6 = File1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected initialization with value of type 'gsl::owner<>'; got 'FILE *' (aka 'int *')
+
+  FILE *File7 = tmpfile();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+  gsl::owner<FILE *> File8 = tmpfile(); // Ok
+
+  nonOwningCall(::fopen("test_name.txt", "r"));
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+  nonOwningCall(std::fopen("test_name.txt", "r"));
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+
+  consumesResource(::fopen("test_name.txt", "r")); // Ok
+
+  int *HeapPointer3 = (int *)aligned_alloc(16ul, 4ul * 32ul);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
+  gsl::owner<int *> HeapPointer4 = static_cast<int *>(aligned_alloc(16ul, 4ul * 32ul)); // Ok
+
+  void *HeapPointer5 = calloc(10ul, 4ul);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>'
+  gsl::owner<void *> HeapPointer6 = calloc(10ul, 4ul); // Ok
+}
+
+void test_legacy_consumers() {
+  int StackInteger = 42;
+
+  int *StackPointer = &StackInteger;
+  int *HeapPointer1 = (int *)malloc(100);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
+  gsl::owner<int *> HeapPointer2 = (int *)malloc(100);
+
+  std::free(StackPointer);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
+  std::free(HeapPointer1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
+  std::free(HeapPointer2); // Ok
+  // CHECK MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
+
+  // FIXME: the check complains about initialization of 'void *' with new created owner.
+  // This happens, because the argument of `free` is not marked as 'owner<>' (and cannot be),
+  // and the check will not figure out could be meant as owner.
+  // This property will probably never be fixed, because it is probably a rather rare
+  // use-case and 'owner<>' should be wrapped in RAII classes anyway!
+  std::free(std::malloc(100)); // Ok but silly :)
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>'
+
+  // Demonstrate, that multi-argument functions are diagnosed as well.
+  std::realloc(StackPointer, 200);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
+  std::realloc(HeapPointer1, 200);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
+  std::realloc(HeapPointer2, 200);     // Ok
+  std::realloc(std::malloc(100), 200); // Ok but silly
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>'
+
+  fclose(fileFactory1());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
+  fclose(fileFactory2()); // Ok, same as FIXME with `free(malloc(100))` applies here
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+
+  gsl::owner<FILE *> File1 = fopen("testfile.txt", "r"); // Ok
+  FILE *File2 = freopen("testfile.txt", "w", File1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+  // CHECK-MESSAGES: [[@LINE-2]]:17: warning: calling legacy resource function without passing a 'gsl::owner<>'
+  // FIXME: The warning for not passing and owner<> is a false positive since both the filename and the
+  // mode are not supposed to be owners but still pointers. The check is to coarse for
+  // this function. Maybe `freopen` gets special treatment.
+
+  gsl::owner<FILE *> File3 = freopen("testfile.txt", "w", File2); // Bad, File2 no owner
+  // CHECK-MESSAGES: [[@LINE-1]]:30: warning: calling legacy resource function without passing a 'gsl::owner<>'
+
+  FILE *TmpFile = tmpfile();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+  FILE *File6 = freopen("testfile.txt", "w", TmpFile); // Bad, both return and argument
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
+  // CHECK-MESSAGES: [[@LINE-2]]:17: warning: calling legacy resource function without passing a 'gsl::owner<>'
+}