Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG7f1963dc8e23: [libc++] Move pointer safety related utilities out of <memory>
rG916fecb499c5: [libc++] Split std::shared_ptr & friends out of <memory>
rG21d6636d83b3: [libc++] Split std::unique_ptr out of <memory>
rG4f9b2469f33f: [libc++] Split the memory-related algorithms out of <memory>
rGbe54341cd2ff: [libc++] Split std::raw_storage_iterator out of <memory>
rG9b0a3388eb36: [libc++] Split __compressed_pair out of <memory>
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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.) |
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). |
@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.
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.