[libcxx-commits] [PATCH] D100318: [libc++] Split various components out of <memory>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D100318#2683513 <https://reviews.llvm.org/D100318#2683513>, @Quuxplusone wrote:

> Here are the dependency graphs before and after this patch.
> At the introduction of the new grapher script, pre- D99309 <https://reviews.llvm.org/D99309>: https://i.imgur.com/wIGKMXG.png (10031 x 1602 pixels, 141 nodes)
> Main, pre-this-patch: https://i.imgur.com/CDUSXcr.png (10360 x 1845 pixels, 141 nodes)
> After this patch: https://i.imgur.com/7mll7PC.png (11243 x 2636 pixels, 150 nodes)

I disagree with the way you calculate complexity. Yes, the graph has more nodes and edges. But each file now contains a set of logically-related things, and is much smaller. As a human, I spend time reading files with code in it, not looking at graphs. The graph is useful as a tool, but it's not measure of success in itself.



================
Comment at: libcxx/include/__memory/gc.h:10-11
+
+#ifndef _LIBCPP___MEMORY_GC_H
+#define _LIBCPP___MEMORY_GC_H
+
----------------
Quuxplusone wrote:
> I suggest (well, I continue to suggest that you not do any of this refactoring, but...) that you name this file `pointer_safety.h`, since `gc.h` is (A) terse and (B) obscure. `pointer_safety.h` would have the benefit of matching a name in the standard (so it's greppable), and also connoting how arcane this stuff is meant to be. `gc.h` sounds like it might be talking about `shared_ptr` or something.
I agree with `pointer_safety.h`, that's a much better name. Thanks.


================
Comment at: libcxx/include/__memory/uninitialized.h:10-11
+
+#ifndef _LIBCPP___MEMORY_UNINITIALIZED_H
+#define _LIBCPP___MEMORY_UNINITIALIZED_H
+
----------------
Quuxplusone wrote:
> `uninitialized_algorithms.h`
Better indeed, thanks.


================
Comment at: libcxx/include/__memory/unique_ptr.h:16
+#include <__memory/allocator_traits.h> // __pointer
+#include <__memory/auto_ptr.h>
+#include <__memory/compressed_pair.h>
----------------
Quuxplusone wrote:
> This is a very surprising edge, since ultimately we want to remove `auto_ptr` altogether. Can you try to remove it? (At the very least, comment why it's here.)
After the move is done, I will go back and make this dependent on the standard version and whether `auto_ptr` is force-enabled. I didn't want to change any code as part of these commits.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100318/new/

https://reviews.llvm.org/D100318