Skip to content

[Clang][NFC] Refactor operator delete argument handling #160554

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 1, 2025
Merged

[Clang][NFC] Refactor operator delete argument handling #160554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ade6f59
[Clang][NFC] Refactor operator delete argument handling
andykaylor Sep 24, 2025
3c185db
Restore original struct and function name
andykaylor Sep 24, 2025
8dfdcb1
Update CIR's emitDeleteCall
andykaylor Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/include/clang/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class TypeAliasTemplateDecl;
class UnresolvedSetImpl;
class VarTemplateDecl;
enum class ImplicitParamKind;
struct UsualDeleteParams;

// Holds a constraint expression along with a pack expansion index, if
// expanded.
Expand Down Expand Up @@ -2646,6 +2647,8 @@ class FunctionDecl : public DeclaratorDecl,
bool isTypeAwareOperatorNewOrDelete() const;
void setIsTypeAwareOperatorNewOrDelete(bool IsTypeAwareOperator = true);

UsualDeleteParams getUsualDeleteParams() const;
Copy link
Contributor

@ojhunt ojhunt Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erichkeane is it reasonable to have this here but the struct definition be in a separate header? Moving the definition to Decl.h, requires a bunch of other related definitions to be moved - I think it would be reasonable to do so if we're going to make this present directly in Decl?

@andykaylor I think this should be an optional return - e.g. std::optional<UsualDeleteParams> or we could smooth out the declaration w/o the definition with bool getUsualDeleteParams(UsualDeleteParams *) const. I'd prefer optional<UsualDeleteParams> given the alternation of true and false meaning success or fail in different places.

Copy link
Collaborator

@erichkeane erichkeane Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a concern with it. Though, you can't really call the function at all without also including ExprCXX.h of course (since it returns a type requiring completeness).

We do this a few other places of course.

The std::optional might be nice, but what state is it supposed to represent? Are you just meaning "for a non-delete decl"? At that point, an assert is probably even better.

Copy link
Contributor Author

@andykaylor andykaylor Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andykaylor I think this should be an optional return - e.g. std::optional<UsualDeleteParams> or we could smooth out the declaration w/o the definition with bool getUsualDeleteParams(UsualDeleteParams *) const. I'd prefer optional<UsualDeleteParams> given the alternation of true and false meaning success or fail in different places.

So rather than assert and return the default structure if it's called for a non-delete function it would just return std::nullopt?

Copy link
Contributor

@ojhunt ojhunt Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's my preference? Though I realize I was thinking we could remove the individual queries but that would require the addition of an equivalent struct for new - but that might be nicer in general? There are existing structs that are similar and maybe we could make things generally nicer by reducing the number of variations of these structures?

Such a change would not be necessary as part of this PR of course - on the other hand maybe an assertion is perfectly reasonable? That is consistent with lots of other code.

Copy link
Contributor

@ojhunt ojhunt Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andykaylor actually yeah, lets stick with an assertion for now - changing the structure of new/delete queries is a different task from this one. r=me.


/// Compute the language linkage.
LanguageLinkage getLanguageLinkage() const;

Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/AST/ExprCXX.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2342,6 +2342,14 @@ struct ImplicitDeallocationParameters {
SizedDeallocationMode PassSize;
};

/// The parameters to pass to a usual operator delete.
struct UsualDeleteParams {
TypeAwareAllocationMode TypeAwareDelete = TypeAwareAllocationMode::No;
bool DestroyingDelete = false;
bool Size = false;
AlignedAllocationMode Alignment = AlignedAllocationMode::No;
};

/// Represents a new-expression for memory allocation and constructor
/// calls, e.g: "new CXXNewExpr(foo)".
class CXXNewExpr final
Expand Down
47 changes: 47 additions & 0 deletions clang/lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3552,6 +3552,53 @@ void FunctionDecl::setIsTypeAwareOperatorNewOrDelete(bool IsTypeAware) {
getASTContext().setIsTypeAwareOperatorNewOrDelete(this, IsTypeAware);
}

UsualDeleteParams FunctionDecl::getUsualDeleteParams() const {
UsualDeleteParams Params;

// This function should only be called for operator delete declarations.
assert(getDeclName().isAnyOperatorDelete());
if (!getDeclName().isAnyOperatorDelete())
return Params;

const FunctionProtoType *FPT = getType()->castAs<FunctionProtoType>();
auto AI = FPT->param_type_begin(), AE = FPT->param_type_end();
Copy link
Collaborator

@shafik shafik Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does AI and AE stand for? Can we get more descriptive names?

Copy link
Collaborator

@erichkeane erichkeane Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats pre-existing (though different capitalization, but I'll guess it is supposed to be 'args-iterator' and 'args-end'. But thats a guess, and also kinda wrong, since these are parameters not args.

Copy link
Collaborator

@shafik shafik Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure you can squint and say it is existing code but if no one understand what the variable names mean, it probably makes sense to just rename them. Your idea seems plausible.


if (isTypeAwareOperatorNewOrDelete()) {
Params.TypeAwareDelete = TypeAwareAllocationMode::Yes;
assert(AI != AE);
++AI;
}

// The first argument after the type-identity parameter (if any) is
// always a void* (or C* for a destroying operator delete for class
// type C).
++AI;

// The next parameter may be a std::destroying_delete_t.
if (isDestroyingOperatorDelete()) {
assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
Params.DestroyingDelete = true;
assert(AI != AE);
++AI;
}

// Figure out what other parameters we should be implicitly passing.
if (AI != AE && (*AI)->isIntegerType()) {
Params.Size = true;
++AI;
} else
assert(!isTypeAwareAllocation(Params.TypeAwareDelete));

if (AI != AE && (*AI)->isAlignValT()) {
Params.Alignment = AlignedAllocationMode::Yes;
++AI;
} else
assert(!isTypeAwareAllocation(Params.TypeAwareDelete));

assert(AI == AE && "unexpected usual deallocation function parameter");
return Params;
}

LanguageLinkage FunctionDecl::getLanguageLinkage() const {
return getDeclLanguageLinkage(*this);
}
Expand Down
64 changes: 5 additions & 59 deletions clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,60 +210,6 @@ RValue CIRGenFunction::emitCXXMemberOrOperatorCall(
return emitCall(fnInfo, callee, returnValue, args, nullptr, loc);
}

namespace {
/// The parameters to pass to a usual operator delete.
struct UsualDeleteParams {
TypeAwareAllocationMode typeAwareDelete = TypeAwareAllocationMode::No;
bool destroyingDelete = false;
bool size = false;
AlignedAllocationMode alignment = AlignedAllocationMode::No;
};
} // namespace

// FIXME(cir): this should be shared with LLVM codegen
static UsualDeleteParams getUsualDeleteParams(const FunctionDecl *fd) {
UsualDeleteParams params;

const FunctionProtoType *fpt = fd->getType()->castAs<FunctionProtoType>();
auto ai = fpt->param_type_begin(), ae = fpt->param_type_end();

if (fd->isTypeAwareOperatorNewOrDelete()) {
params.typeAwareDelete = TypeAwareAllocationMode::Yes;
assert(ai != ae);
++ai;
}

// The first argument after the type-identity parameter (if any) is
// always a void* (or C* for a destroying operator delete for class
// type C).
++ai;

// The next parameter may be a std::destroying_delete_t.
if (fd->isDestroyingOperatorDelete()) {
params.destroyingDelete = true;
assert(ai != ae);
++ai;
}

// Figure out what other parameters we should be implicitly passing.
if (ai != ae && (*ai)->isIntegerType()) {
params.size = true;
++ai;
} else {
assert(!isTypeAwareAllocation(params.typeAwareDelete));
}

if (ai != ae && (*ai)->isAlignValT()) {
params.alignment = AlignedAllocationMode::Yes;
++ai;
} else {
assert(!isTypeAwareAllocation(params.typeAwareDelete));
}

assert(ai == ae && "unexpected usual deallocation function parameter");
return params;
}

static mlir::Value emitCXXNewAllocSize(CIRGenFunction &cgf, const CXXNewExpr *e,
unsigned minElements,
mlir::Value &numElements,
Expand Down Expand Up @@ -616,11 +562,11 @@ void CIRGenFunction::emitDeleteCall(const FunctionDecl *deleteFD,
const auto *deleteFTy = deleteFD->getType()->castAs<FunctionProtoType>();
CallArgList deleteArgs;

UsualDeleteParams params = getUsualDeleteParams(deleteFD);
UsualDeleteParams params = deleteFD->getUsualDeleteParams();
auto paramTypeIt = deleteFTy->param_type_begin();

// Pass std::type_identity tag if present
if (isTypeAwareAllocation(params.typeAwareDelete))
if (isTypeAwareAllocation(params.TypeAwareDelete))
cgm.errorNYI(deleteFD->getSourceRange(),
"emitDeleteCall: type aware delete");

Expand All @@ -631,12 +577,12 @@ void CIRGenFunction::emitDeleteCall(const FunctionDecl *deleteFD,
deleteArgs.add(RValue::get(deletePtr), argTy);

// Pass the std::destroying_delete tag if present.
if (params.destroyingDelete)
if (params.DestroyingDelete)
cgm.errorNYI(deleteFD->getSourceRange(),
"emitDeleteCall: destroying delete");

// Pass the size if the delete function has a size_t parameter.
if (params.size) {
if (params.Size) {
QualType sizeType = *paramTypeIt++;
CharUnits deleteTypeSize = getContext().getTypeSizeInChars(deleteTy);
assert(mlir::isa<cir::IntType>(convertType(sizeType)) &&
Expand All @@ -648,7 +594,7 @@ void CIRGenFunction::emitDeleteCall(const FunctionDecl *deleteFD,
}

// Pass the alignment if the delete function has an align_val_t parameter.
if (isAlignedAllocation(params.alignment))
if (isAlignedAllocation(params.Alignment))
cgm.errorNYI(deleteFD->getSourceRange(),
"emitDeleteCall: aligned allocation");

Expand Down
56 changes: 2 additions & 54 deletions clang/lib/CodeGen/CGExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1376,58 +1376,6 @@ RValue CodeGenFunction::EmitBuiltinNewDeleteCall(const FunctionProtoType *Type,
llvm_unreachable("predeclared global operator new/delete is missing");
}

namespace {
/// The parameters to pass to a usual operator delete.
struct UsualDeleteParams {
TypeAwareAllocationMode TypeAwareDelete = TypeAwareAllocationMode::No;
bool DestroyingDelete = false;
bool Size = false;
AlignedAllocationMode Alignment = AlignedAllocationMode::No;
};
}

static UsualDeleteParams getUsualDeleteParams(const FunctionDecl *FD) {
UsualDeleteParams Params;

const FunctionProtoType *FPT = FD->getType()->castAs<FunctionProtoType>();
auto AI = FPT->param_type_begin(), AE = FPT->param_type_end();

if (FD->isTypeAwareOperatorNewOrDelete()) {
Params.TypeAwareDelete = TypeAwareAllocationMode::Yes;
assert(AI != AE);
++AI;
}

// The first argument after the type-identity parameter (if any) is
// always a void* (or C* for a destroying operator delete for class
// type C).
++AI;

// The next parameter may be a std::destroying_delete_t.
if (FD->isDestroyingOperatorDelete()) {
assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
Params.DestroyingDelete = true;
assert(AI != AE);
++AI;
}

// Figure out what other parameters we should be implicitly passing.
if (AI != AE && (*AI)->isIntegerType()) {
Params.Size = true;
++AI;
} else
assert(!isTypeAwareAllocation(Params.TypeAwareDelete));

if (AI != AE && (*AI)->isAlignValT()) {
Params.Alignment = AlignedAllocationMode::Yes;
++AI;
} else
assert(!isTypeAwareAllocation(Params.TypeAwareDelete));

assert(AI == AE && "unexpected usual deallocation function parameter");
return Params;
}

namespace {
/// A cleanup to call the given 'operator delete' function upon abnormal
/// exit from a new expression. Templated on a traits type that deals with
Expand Down Expand Up @@ -1505,7 +1453,7 @@ namespace {
} else {
// For a non-placement new-expression, 'operator delete' can take a
// size and/or an alignment if it has the right parameters.
Params = getUsualDeleteParams(OperatorDelete);
Params = OperatorDelete->getUsualDeleteParams();
}

assert(!Params.DestroyingDelete &&
Expand Down Expand Up @@ -1838,7 +1786,7 @@ void CodeGenFunction::EmitDeleteCall(const FunctionDecl *DeleteFD,
const auto *DeleteFTy = DeleteFD->getType()->castAs<FunctionProtoType>();
CallArgList DeleteArgs;

auto Params = getUsualDeleteParams(DeleteFD);
auto Params = DeleteFD->getUsualDeleteParams();
auto ParamTypeIt = DeleteFTy->param_type_begin();

std::optional<llvm::AllocaInst *> TagAlloca;
Expand Down