I think we need to do the opposite thing: guard sized delete with feature check. Note that -fsized-deallocation is an ABI break, so it might not be appropriate for some downstream users.

It looks like this is a C++14 feature but Clang has neglected to implement it:
https://reviews.llvm.org/D112921

So it should definitely be the default behavior. We could add a feature flag for folks that want to keep it off but I am not sure why they would want that.

Maybe we do not even need to add a sized delete and a simple delete is sufficient:
https://developers.redhat.com/blog/2021/05/05/detecting-memory-management-bugs-with-gcc-11-part-2-deallocation-functions#mismatches_between_new_and_delete_operators

I do not have GCC 11 at hand.

This mostly comes does to what C++ version are we using? We'd previously made a decision to try to converge on C++20, so we should use whatever flags are needed to get the compiler to support C++20 if at all possible. If we find compilers that really cannot support C++20, then we can put in checks/workarounds in the code for those compilers.

We should be detecting which versions of clang need this flag to support C++20, and only apply it then.

I do not see why we need to do any version detection, note that apple clang and mainline clang are different clangs, and any version checks are fragile by definition. Even more, sized deallocation could be a platform choice (Apple platforms including).

We can just check via #ifdef __cpp_sized_deallocation and provide sized delete's only when the feature is enabled.

If platforms that follow C++20 consistently set __cpp_sized_deallocation and those that don't follow C++20 don't set it, that would be fine. But I can't find any mention of this preprocessor macro in any version of the C++ spec.

These are feature testing macro and they are essentially implementation defined: https://en.cppreference.com/w/cpp/feature_test

FWIW: clang recently enabled -fsized-deallocation by default: llvm/llvm-project#90373 so this is part of LLVM 19 release and further on. Likely this will be default in Apple fork in 1-2 years, however, it will certainly be SDK version-specific (e.g. enabling compatibility with lower SDK versions would default to -fnosized-deallocation and newer ones will be -fsized-deallocation) due to ABI compatibility

One example of check could be seen at https://github.com/google/cel-cpp/pull/558/files:

-      ::operator delete(static_cast<void*>(p), n * sizeof(T),
+#if defined(__cpp_sized_deallocation) && __cpp_sized_deallocation >= 201309L
+      ::operator delete(static_cast<void *>(p), n * sizeof(T),
+                        static_cast<std::align_val_t>(alignof(T)));
+#else
+      ::operator delete(static_cast<void *>(p),
+                        static_cast<std::align_val_t>(alignof(T)));
+      static_cast<void>(n); // unused
+#endif

Similar code is used on Boost to make it compatible (https://www.boost.org/doc/libs/1_84_0/boost/container/new_allocator.hpp), jemalloc, libc++, etc.

I agree that __cpp_sized_deallocation should be the way to go unless it is broken in some of the compilers which we target. This should be much more robust than checking compiler version.

Out of curiosity, how is this related to C++20? The sized dealloc was added in C++14.

Yeah. Does not seem related to me.

Based on the discussion in https://reviews.llvm.org/D112921 I gather that recent versions of clang will support size deallocation. Once we switch to C++20 and upgrade our compilers it will be worth revisiting whether we still need this. The C++20 is just an easily greppable tag.

The problem being compilers that claim to support a certain version of C++ (14 or 20) but don't completely -- they're missing some features. The __cpp_FETAURE macros themselves are part of C++23, so there exists the possibility of a C++ compiler that supports the feature but does not define the macro, but hopefully that will be rare.

Note that we could just use #if __cpp_sized_deallocation >= 201309L as the preprocessor defines any identifier that is not a macro evaluates to 0 in an #if

I decided to leave it as defined(__cpp_sized_deallocation) && __cpp_sized_deallocation >= 201309L to make this check a bit more explicit.

Based on the discussion in https://reviews.llvm.org/D112921 I gather that recent versions of clang will support size deallocation. Once we switch to C++20 and upgrade our compilers it will be worth revisiting whether we still need this. The C++20 is just an easily greppable tag.

This is enabled by default in mainline clang as of LLVM 19. However, sized deallocation is a platform choice. E.g. on Apple downstream clang fork it will definitely be availability-tied and enabled only in certain SDK versions.

Possibly accidental?

Do we really need all these cmake changes? I think they are obsolete after flag was removed

Do we really need all these cmake changes? I think they are obsolete after flag was removed

They actually fixed an issue where options set in the ir lib are not propagated correctly to downstream dependencies. I decided to keep them in but I can also move them to a separate PR.