This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Split various components out of <memory>
ClosedPublic

Authored by ldionne on Apr 12 2021, 8:51 AM.

Diff Detail

Event Timeline

ldionne created this revision.Apr 12 2021, 8:51 AM
ldionne requested review of this revision.Apr 12 2021, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 8:51 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Here are the dependency graphs before and after this patch.
At the introduction of the new grapher script, pre- 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)

libcxx/include/__memory/gc.h
10–11

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.

libcxx/include/__memory/uninitialized.h
10–11

uninitialized_algorithms.h

libcxx/include/__memory/unique_ptr.h
16

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.)

ldionne updated this revision to Diff 336905.Apr 12 2021, 11:01 AM
ldionne marked 2 inline comments as done.

Fix failing tests, rename headers as suggested by reviewers.

Here are the dependency graphs before and after this patch.
At the introduction of the new grapher script, pre- 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.

libcxx/include/__memory/gc.h
10–11

I agree with pointer_safety.h, that's a much better name. Thanks.

libcxx/include/__memory/uninitialized.h
10–11

Better indeed, thanks.

libcxx/include/__memory/unique_ptr.h
16

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.

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.

Me too! ;) When I'm reading that code, though, I tend to stumble over two measures of complexity:
(1) How hard do I have to look to find the source file that actually contains the thing I'm working on? (Once the right file is open, I can jump to the right line quickly; but I don't use an IDE that can jump me to the right file quickly.)
(2) How tightly entangled is this source file with other source files? (How many other files do I need to read in order to fully understand the code in this file?)
These two measures of complexity map directly onto "number of nodes in the graph" and "number of edges in the graph."
I do think minimizing edges is generally more important than minimizing nodes (and of course it's most important to keep the number of cycles at 0). Minimizing the diameter of the graph (the number of levels of abstraction you have to dig through to work on the code) is also relevant, if maybe less important than the other measures listed here.

libcxx/include/__memory/unique_ptr.h
16

Ah, I get it now — the culprit is literally the constructor unique_ptr(auto_ptr<Y>&&), which is removed only in C++20. Okay (although I'd still prefer to see the code comment // std::auto_ptr for clarity).

ldionne updated this revision to Diff 336965.Apr 12 2021, 2:26 PM

Workaround failures on Clang 10

@DavidSpickett Would it be possible to upgrade the ARM bots to Clang 11? They are currently running Clang 10, which is still supported for now, however it is missing 05eedf1f5b44, which makes some tests fail: https://buildkite.com/llvm-project/libcxx-ci/builds/2462#d220f5f2-25ed-498b-88e3-0013c0b5e3a1.

Arm bots are now on clang 11.1.0.

ldionne accepted this revision.Apr 13 2021, 5:20 AM

Arm bots are now on clang 11.1.0.

Thanks for the prompt reply!

This revision is now accepted and ready to land.Apr 13 2021, 5:20 AM
libcxx/include/__memory/unique_ptr.h