[PATCH] More on adding sized deallocation functions in libc++

[Larisse Voufo]

I am uploading an update in a few minutes...


================
Comment at: src/new.cpp:134
@@ -133,2 +133,3 @@
 
+#if _LIBCPP_STD_VER >= 14
 _LIBCPP_WEAK _LIBCPP_NEW_DELETE_VIS
----------------
mclow.lists wrote:
> rsmith wrote:
> > Remove this `#if`/`#endif` pair and define the weak version unconditionally. The contents of the standard library should not depend on the `-std=` flag passed when bulding it. Because this function is weak, this won't break valid C++11 and earlier code that happens to be defining such a function.
> Exactly correct. There should be no requirement to coordinate the `-std=` flag used to build the library and the flag used to build programs that use the library.
> 
Done.

================
Comment at: src/new.cpp:134
@@ -133,2 +133,3 @@
 
+#if _LIBCPP_STD_VER >= 14
 _LIBCPP_WEAK _LIBCPP_NEW_DELETE_VIS
----------------
lvoufo wrote:
> mclow.lists wrote:
> > rsmith wrote:
> > > Remove this `#if`/`#endif` pair and define the weak version unconditionally. The contents of the standard library should not depend on the `-std=` flag passed when bulding it. Because this function is weak, this won't break valid C++11 and earlier code that happens to be defining such a function.
> > Exactly correct. There should be no requirement to coordinate the `-std=` flag used to build the library and the flag used to build programs that use the library.
> > 
> Done.
Ok.

================
Comment at: src/new.cpp:164
@@ -161,2 +163,3 @@
 
+#if _LIBCPP_STD_VER >= 14
 _LIBCPP_WEAK _LIBCPP_NEW_DELETE_VIS
----------------
rsmith wrote:
> Likewise.
Done.

================
Comment at: test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array.pass.cpp:53
@@ +52,3 @@
+    assert(!delete_nothrow_called);
+    delete [] ap;
+    assert(!A_constructed);
----------------
rsmith wrote:
> This is not guaranteed to work; your calls to `operator new` and `operator delete` are elidable. Directly call `operator delete` here. (That also lets you run this test in all language modes, not just C++14 mode.)
Ok, on the extension of the test in all language modes.
As far as guaranteeing that this works, I would like to keep these passes 
(1) consistent with the current design (in {new, new_nothrow, new_array, new_array_nothrow}.pass.cpp), and
(2) more focused on changes in `delete` than in `new`, 
(3) as resulting from using new/delete expressions rather than new/delete operator function calls (which, as confirmed by some quick tests, happen to follow different lookup paths).
So, I think I'll just define a new handler similarly to the passes in (1). 

This incidentally highlights something we may have missed in the standard on sized nothrow deallocation (see  FIXMEs on related test cases below).


================
Comment at: test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array_nothrow.pass.cpp:29
@@ +28,3 @@
+  ++delete_called;
+  std::free(p);
+}
----------------
mclow.lists wrote:
> rsmith wrote:
> > It's questionable to assume that memory allocated by libc++'s `operator new` can be freed by `std::free`. I would suggest either replacing `operator new[]` too, or just leaking the memory.
> I would prefer replacing `operator new[]` as well.
For the reasons I gave above, I would rather provide a new handler, similarly to the current tests' design, than replace `new`.

================
Comment at: test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array_nothrow.pass.cpp:48
@@ +47,3 @@
+{
+    A* ap = new A(std::nothrow) [3];
+    assert(ap);
----------------
mclow.lists wrote:
> Shouldn't this be `A* ap = new (std::nothrow) A [3];` ??
Right. I find it strange that the libcxx test system did not catch this error, and several others below. I actually took a closer and replaced the value of 'config.std' in my test/lit.site.cfg file from "c++11" to "c++1y".
Then, running make check-libcxx with this change led to the following unexpected failures.
I wonder if these might happen to be false negatives somehow... How do we normally test "c++14" mode in libcxx?

Failing Tests (11):
    libc++ :: std/experimental/string.view/string.view.find/find_last_not_of_char_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.find/find_last_not_of_pointer_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.find/find_last_not_of_pointer_size_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.find/find_last_of_pointer_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.find/find_last_of_pointer_size_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.find/rfind_pointer_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.find/rfind_pointer_size_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.find/rfind_string_view_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.ops/compare.pointer_size.pass.cpp
    libc++ :: std/experimental/string.view/string.view.ops/compare.size_size_sv_pointer_size.pass.cpp
    libc++ :: std/experimental/utilities/time/header.chrono.synop/treat_as_floating_point_v.pass.cpp

http://reviews.llvm.org/D7707

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/