[libc++] Avoid including vector in <functional>

[philnik777]

vector has been used in a very simple way in boyer_moore_searcher. We can instead just use unique_ptr<T[]>, which is a lot simpler, allowing us to drop the vector dependency while not losing any expressiveness in the code. As a nice side effect, this also reduces the time it takes to instantiate the boyer_moore_searcher constructor from 26ms to 22ms on my machine.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

vector has been used in a very simple way in boyer_moore_searcher. We can instead just use unique_ptr<T[]>, which is a lot simpler, allowing us to drop the vector dependency while not losing any expressiveness in the code. As a nice side effect, this also reduces the time it takes to instantiate the boyer_moore_searcher constructor from 26ms to 22ms on my machine.

---

Full diff: https://github.com/llvm/llvm-project/pull/144310.diff

1 Files Affected:

• (modified) libcxx/include/__functional/boyer_moore_searcher.h (+1-3)

diff --git a/libcxx/include/__functional/boyer_moore_searcher.h b/libcxx/include/__functional/boyer_moore_searcher.h
index 7889232f4b919..88c6577f04dee 100644
--- a/libcxx/include/__functional/boyer_moore_searcher.h
+++ b/libcxx/include/__functional/boyer_moore_searcher.h
@@ -17,12 +17,10 @@
 #include <__config>
 #include <__functional/hash.h>
 #include <__functional/operations.h>
-#include <__iterator/distance.h>
 #include <__iterator/iterator_traits.h>
 #include <__memory/shared_ptr.h>
 #include <__type_traits/make_unsigned.h>
 #include <__utility/pair.h>
-#include <__vector/vector.h>
 #include <array>
 #include <limits>
 #include <unordered_map>
@@ -196,7 +194,7 @@ class boyer_moore_searcher {
     if (__count == 0)
       return;
 
-    vector<difference_type> __scratch(__count);
+    auto __scratch = std::make_unique<difference_type[]>(__count);
 
     __compute_bm_prefix(__first, __last, __pred, __scratch);
     for (size_t __i = 0; __i <= __count; ++__i)

[ldionne]

LGTM. After verification, we already include <vector> in <functional> for transitive-includes stability, so this shouldn't have user impact.

[philnik777]

@ldionne Just so you know, this will affect C++23. This is an example why I think #134143 wouldn't actually help much. I think this will be way more common than removing public headers.

[ldionne]

@ldionne Just so you know, this will affect C++23. This is an example why I think #134143 wouldn't actually help much. I think this will be way more common than removing public headers.

Ah, you are right, I missed that the block was guarded like this:

#  if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20

<= 20 means that this doesn't apply to C++23.

In that case, I do have an issue with this patch as-is. It would be trivial for us to keep including <vector> from <functional> in C++23 (which is a stable standard mode) to avoid breaking people.

We can then be intentional about dropping these transitive includes when we so desire, with a plan. I'm in favour of dropping transitive includes and forcing people to include what they use. I have little sympathy for keeping tech debt around just for the sake of not roughing feathers. However, I also think that we need to be intentional about what we break and when, otherwise we're expanding important "user sympathy" capital by breaking people often and for a questionable benefit.

I'd much rather ship #134143 (perhaps with a rewording that explains the intent better now that this goes beyond just checking transitive public includes), and then add #include <vector> to this patch in C++23 mode.

[philnik777]

@ldionne Just so you know, this will affect C++23. This is an example why I think #134143 wouldn't actually help much. I think this will be way more common than removing public headers.

Ah, you are right, I missed that the block was guarded like this:

#  if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20

<= 20 means that this doesn't apply to C++23.

In that case, I do have an issue with this patch as-is. It would be trivial for us to keep including <vector> from <functional> in C++23 (which is a stable standard mode) to avoid breaking people.

What do you mean by "stable standard mode"?

We can then be intentional about dropping these transitive includes when we so desire, with a plan. I'm in favour of dropping transitive includes and forcing people to include what they use. I have little sympathy for keeping tech debt around just for the sake of not roughing feathers. However, I also think that we need to be intentional about what we break and when, otherwise we're expanding important "user sympathy" capital by breaking people often and for a questionable benefit.

I'd much rather ship #134143 (perhaps with a rewording that explains the intent better now that this goes beyond just checking transitive public includes), and then add #include <vector> to this patch in C++23 mode.

I don't think that's a viable path forward. You're basically asking to double the include time of <functional> to make things work with our current tests to avoid removing includes. IMO that's not something we should ever do.

[ldionne]

In that case, I do have an issue with this patch as-is. It would be trivial for us to keep including <vector> from <functional> in C++23 (which is a stable standard mode) to avoid breaking people.

What do you mean by "stable standard mode"?

I mean that C++23 has been released by ISO and we consider it a standard mode that people can write production code with. That is unlike e.g. C++26 where things could still technically change since it hasn't been ratified yet.

I don't think that's a viable path forward. You're basically asking to double the include time of <functional> to make things work with our current tests to avoid removing includes. IMO that's not something we should ever do.

I'm not asking to double the include time of <functional> -- it including <vector> is the status quo. I am saying that we should not be making changes like this one that we know for a fact will break many users (in a trivial way, sure) when we have an easy way not to. Instead, we should be intentional about these breaks.

For example: we want to drop all of the transitive includes every other release? Sure, that's something we can try out. Or just once in the next release? Also something we can try out. But we should have proper communication and we should do it intentionally. Otherwise, we are just creating constant churn for users who are trying to update (or have to update), and the benefit is something that many of them just don't care about.

I don't want to make it more difficult for the library to move forward and make these kinds of improvements. But I want to have a principled way of moving forward with changes that we know for a fact will break people.

[philnik777]

I don't think that's a viable path forward. You're basically asking to double the include time of <functional> to make things work with our current tests to avoid removing includes. IMO that's not something we should ever do.

I'm not asking to double the include time of <functional> -- it including <vector> is the status quo. I am saying that we should not be making changes like this one that we know for a fact will break many users (in a trivial way, sure) when we have an easy way not to. Instead, we should be intentional about these breaks.

I'm not sure if we're just talking past each other here. The status quo is that <__vector/vector.h> is included. Do you mean that? If you actually mean <vector>, that is not the status quo. #include <functional> #include <vector> takes almost twice as long as #include <functional> in C++23 in trunk.

For example: we want to drop all of the transitive includes every other release? Sure, that's something we can try out. Or just once in the next release? Also something we can try out. But we should have proper communication and we should do it intentionally. Otherwise, we are just creating constant churn for users who are trying to update (or have to update), and the benefit is something that many of them just don't care about.

I don't want to make it more difficult for the library to move forward and make these kinds of improvements. But I want to have a principled way of moving forward with changes that we know for a fact will break people.

I absolutely agree that we should be intentional. My concern is that we currently have no tool to ensure we're not accidentally removing features from C++23. Previous modes are much better off because they weren't granular from the start, making the current "don't drop top-level headers" pretty good. That's not a viable approach anymore as this and other patches show.