Skip to content

Commit

Permalink
Improve handling of placement new
Browse files Browse the repository at this point in the history
Instead of 'IsDefaultNewOrDelete' which uses heuristics and string
parsing, use FunctionDecl::isReplaceableGlobalAllocationFunction from
the Clang API. It returns true for any replaceable allocation function,
which happens to coincide nicely with all allocation functions except
placement new.

Fixes #777 and improves behavior for the modern allocation functions:

- Sized deallocation in C++14
- Aligned allocation in C++17

This small improvement fixes a number of TODOs in the new placement_new
testcase, and makes it possible to add a testcase for implicit aligned
allocation in operator_new, as aligned allocation no longer requires
<new>.

Patch based on work and lots of good input from Adar Dembo.
  • Loading branch information
kimgr committed May 2, 2020
1 parent 03ace07 commit a85cea1
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 42 deletions.
24 changes: 0 additions & 24 deletions iwyu_ast_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "iwyu_path_util.h"
#include "iwyu_port.h" // for CHECK_
#include "iwyu_stl_util.h"
#include "iwyu_string_util.h"
#include "iwyu_verrs.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/Casting.h"
Expand Down Expand Up @@ -937,29 +936,6 @@ const NamedDecl* GetDefinitionAsWritten(const NamedDecl* decl) {
return decl;
}

bool IsDefaultNewOrDelete(const FunctionDecl* decl,
const string& decl_loc_as_quoted_include) {
// Clang will report <new> as the location of the default new and
// delete operators if <new> is included. Otherwise, it reports the
// (fake) file "<built-in>".
if (decl_loc_as_quoted_include != "<new>" &&
!IsBuiltinFile(GetFileEntry(decl)))
return false;

const string decl_name = decl->getNameAsString();
if (!StartsWith(decl_name, "operator new") &&
!StartsWith(decl_name, "operator delete"))
return false;

// Placement-new/delete has 2 args, second is void*. The only other
// 2-arg overloads of new/delete in <new> take a const nothrow_t&.
if (decl->getNumParams() == 2 &&
!decl->getParamDecl(1)->getType().isConstQualified())
return false;

return true;
}

bool IsFriendDecl(const Decl* decl) {
// For 'template<...> friend class T', the decl will just be 'class T'.
// We need to go 'up' a level to check friendship in the right place.
Expand Down
6 changes: 0 additions & 6 deletions iwyu_ast_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -584,12 +584,6 @@ const clang::NamedDecl* GetInstantiatedFromDecl(
// the original input.
const clang::NamedDecl* GetDefinitionAsWritten(const clang::NamedDecl* decl);

// True if this decl is for default (not placement-new)
// new/delete/new[]/delete[] from <new>. The second argument
// is the quoted form of the file the decl comes from, e.g. '<new>'.
bool IsDefaultNewOrDelete(const clang::FunctionDecl* decl,
const string& decl_loc_as_quoted_include);

// Returns true if this decl is part of a friend decl.
bool IsFriendDecl(const clang::Decl* decl);

Expand Down
10 changes: 7 additions & 3 deletions iwyu_output.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1242,10 +1242,14 @@ void ProcessFullUse(OneUse* use,
return;
}
// Special case for operators new/delete: Only treated as built-in if they
// are the default, non-placement versions.
// are the default, non-placement versions. This is modelled in Clang as
// 'replaceable global allocation functions': the helper method returns true
// for anything but placement-new. Users of the 'std::nothrow' and
// 'std::align_val_t' overloads already need to spell these two symbols, so
// <new> will be required for them without us doing any magic for operator new
// itself.
if (const FunctionDecl* fn_decl = DynCastFrom(use->decl())) {
const string dfn_file = GetFilePath(fn_decl);
if (IsDefaultNewOrDelete(fn_decl, ConvertToQuotedInclude(dfn_file))) {
if (fn_decl->isReplaceableGlobalAllocationFunction(nullptr, nullptr)) {
VERRS(6) << "Ignoring use of " << use->symbol_name()
<< " (" << use->PrintableUseLoc() << "): built-in new/delete\n";
use->set_ignore_use();
Expand Down
16 changes: 16 additions & 0 deletions tests/cxx/operator_new.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,22 @@ void ExpressionsUserTypes() {
delete[] arr;
}

// Aligned allocation uses operator new(size_t, std::align_val_t) under the
// hood in C++17, but does not require <new> to be included for it. Pre-C++17,
// the alignment is silently ignored (or unsupported if the standard library
// does not support aligned allocation).
void ImplicitAlignedAllocation() {
struct alignas(32) Aligned {
float value[8];
};

Aligned* elem = new Aligned;
delete elem;

Aligned* arr = new Aligned[10];
delete[] arr;
}

/**** IWYU_SUMMARY
tests/cxx/operator_new.cc should add these lines:
Expand Down
10 changes: 1 addition & 9 deletions tests/cxx/placement_new.cc
Original file line number Diff line number Diff line change
Expand Up @@ -96,20 +96,16 @@ void PlacementNewOfTemplate() {
// To use 'std::nothrow' we must include <new>, even if we don't need it for
// 'new' itself.
void NoThrow() {
// TODO: <new> should not be required for operator new here.
// IWYU: IndirectClass needs a declaration
// IWYU: IndirectClass is...*indirect.h
// IWYU: std::nothrow is...*<new>
// IWYU: operator new is...*<new>
IndirectClass* elem = new (std::nothrow) IndirectClass;
// IWYU: IndirectClass is...*indirect.h
delete elem;

// TODO: <new> should not be required for operator new[] here.
// IWYU: IndirectClass needs a declaration
// IWYU: IndirectClass is...*indirect.h
// IWYU: std::nothrow is...*<new>
// IWYU: operator new\[\] is...*<new>
IndirectClass* arr = new (std::nothrow) IndirectClass[4];
// IWYU: IndirectClass is...*indirect.h
delete[] arr;
Expand All @@ -121,20 +117,16 @@ void NoThrow() {
// for 'new' itself.
// The aligned allocation mechanics are only available as of C++17.
void ExplicitAlignedAllocation() {
// TODO: <new> should not be required for operator new here.
// IWYU: IndirectClass needs a declaration
// IWYU: IndirectClass is...*indirect.h
// IWYU: std::align_val_t is...*<new>
// IWYU: operator new is...*<new>
IndirectClass* elem = new (std::align_val_t(32)) IndirectClass;
// IWYU: IndirectClass is...*indirect.h
delete elem;

// TODO: <new> should not be required for operator new[] here.
// IWYU: IndirectClass needs a declaration
// IWYU: IndirectClass is...*indirect.h
// IWYU: std::align_val_t is...*<new>
// IWYU: operator new\[\] is...*<new>
IndirectClass* arr = new (std::align_val_t(32)) IndirectClass[10];
// IWYU: IndirectClass is...*indirect.h
delete[] arr;
Expand All @@ -152,7 +144,7 @@ tests/cxx/placement_new.cc should remove these lines:
- #include "tests/cxx/placement_new-d1.h" // lines XX-XX
The full include-list for tests/cxx/placement_new.cc:
#include <new> // for align_val_t, nothrow, operator new, operator new[]
#include <new> // for align_val_t, nothrow, operator new
#include "tests/cxx/indirect.h" // for IndirectClass
#include "tests/cxx/placement_new-i1.h" // for ClassTemplate
Expand Down

0 comments on commit a85cea1

Please sign in to comment.