-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
[clang] Enable sized deallocation by default in C++14 onwards #83774
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -7294,10 +7294,15 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA, | |||||||||
Args.addOptInFlag(CmdArgs, options::OPT_frelaxed_template_template_args, | ||||||||||
options::OPT_fno_relaxed_template_template_args); | ||||||||||
|
||||||||||
// -fsized-deallocation is off by default, as it is an ABI-breaking change for | ||||||||||
// most platforms. | ||||||||||
Args.addOptInFlag(CmdArgs, options::OPT_fsized_deallocation, | ||||||||||
options::OPT_fno_sized_deallocation); | ||||||||||
// -fsized-deallocation is on by default in C++14 onwards and otherwise off | ||||||||||
// by default. | ||||||||||
if (Arg *A = Args.getLastArg(options::OPT_fsized_deallocation, | ||||||||||
options::OPT_fno_sized_deallocation)) { | ||||||||||
if (A->getOption().matches(options::OPT_fno_sized_deallocation)) | ||||||||||
CmdArgs.push_back("-fno-sized-deallocation"); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not pass -fsized-deallocation to cc1, as -fno-sized-deallocation is not a cc1 option and will cause a failure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if I understand correctly, but I think we need both clang and cc1 options for llvm-project/clang/include/clang/Driver/Options.td Lines 3355 to 3358 in 24e8c6a
And there are a lot of tests assumed we have cc1 options -fno-sized-deallocation and -fsized-deallocation like clang/test/SemaCXX/builtin-operator-new-delete.cpp .
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for bothering you, but I'd like to know if it makes sense to you. @MaskRay There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. There are many -fsize-deallocation in existing tests. I can clean up them this patch lands. |
||||||||||
else | ||||||||||
CmdArgs.push_back("-fsized-deallocation"); | ||||||||||
} | ||||||||||
|
||||||||||
// -faligned-allocation is on by default in C++17 onwards and otherwise off | ||||||||||
// by default. | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need
-fno-sized-deallocation
for CC1Option. When neither option is specified, assume implicit-fsized-deallocation
.The code change in Darwin.cpp should be changed to a function that decides whether the default is yes or no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I may not understand what you mean, can I leave it unchanged as this code is just like
-faligned-allocation
. We can change it later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment. I think this should be fixed.