Making span.h available within partition_alloc. [chromium/src : main]
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 13 comments![]( "Open in Gerrit")
#include "partition_alloc/partition_alloc_base/containers/span.h"Is this needed?
// Copyright 2012 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef BASE_BASE_EXPORT_H_
#define BASE_BASE_EXPORT_H_
#if defined(COMPONENT_BUILD)
#if defined(WIN32)
#if defined(BASE_IMPLEMENTATION)
#define BASE_EXPORT __declspec(dllexport)
#else
#define BASE_EXPORT __declspec(dllimport)
#endif // defined(BASE_IMPLEMENTATION)
#else // defined(WIN32)
#define BASE_EXPORT __attribute__((visibility("default")))
#endif
#else // defined(COMPONENT_BUILD)
#define BASE_EXPORT
#endif
#endif // BASE_BASE_EXPORT_H_
We should reuse:
```
PA_COMPONENT_EXPORT(PARTITION_ALLOC)
```
and #include #include "partition_alloc/partition_alloc_base/component_export.h"
namespace base {This should be `partition_alloc::internal::base`
#include "base/check.h"
#include "base/compiler_specific.h"
#include "base/containers/span_forward_internal.h"
#include "base/memory/raw_ptr_exclusion.h"We shouldn't depend on "base".
we can use:
- `partition_alloc/partition_alloc_base/check.h`
- `span_forward_internal.h` -> To be added.
- `partition_alloc/pointers/raw_ptr_exclusion.h`
- `partition_alloc/build_config.h`
#include "base/check.h"
#include "base/compiler_specific.h"
#include "base/containers/checked_iterators.h"
#include "base/containers/span_forward_internal.h"
#include "base/numerics/integral_constant_like.h"
#include "base/numerics/safe_conversions.h"
#include "base/types/to_address.h"Ditto: we can depend on "base". We can only depend on "partition_alloc", so we need to find alternatives to those files.
// Copyright 2020 The Chromium AuthorsThis already exist as:
base/allocator/partition_allocator/src/partition_alloc/partition_alloc_check.h
and `PA_CHECK` macro
// Copyright 2012 The Chromium AuthorsIs this file needed somewhere? If not, we can revert adding it.
// Copyright 2023 The Chromium AuthorsI believe this file isn't necessary, right?
// Copyright 2023 The Chromium AuthorsI believe this file isn't necessary, right?
// Copyright 2023 The Chromium AuthorsNo need to create this file, because it is already:
"partition_alloc/pointers/raw_ptr_exclusion.h"
// Copyright 2023 The Chromium AuthorsI don't believe this file is used, so we can get rid of it?
// Copyright 2021 The Chromium AuthorsIf we remove `Location`, then this file isn't used anywhere, so it can be removed?
Related details
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano added 12 comments![]( "Open in Gerrit")
#include "partition_alloc/partition_alloc_base/containers/span.h"Sergio SolanoIs this needed?
Done
// Copyright 2012 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef BASE_BASE_EXPORT_H_
#define BASE_BASE_EXPORT_H_
#if defined(COMPONENT_BUILD)
#if defined(WIN32)
#if defined(BASE_IMPLEMENTATION)
#define BASE_EXPORT __declspec(dllexport)
#else
#define BASE_EXPORT __declspec(dllimport)
#endif // defined(BASE_IMPLEMENTATION)
#else // defined(WIN32)
#define BASE_EXPORT __attribute__((visibility("default")))
#endif
#else // defined(COMPONENT_BUILD)
#define BASE_EXPORT
#endif
#endif // BASE_BASE_EXPORT_H_
We should reuse:
```
PA_COMPONENT_EXPORT(PARTITION_ALLOC)
```
and #include #include "partition_alloc/partition_alloc_base/component_export.h"
I'm not sure if this is what you mean.
This should be `partition_alloc::internal::base`
Done
#include "base/check.h"
#include "base/compiler_specific.h"
#include "base/containers/span_forward_internal.h"
#include "base/memory/raw_ptr_exclusion.h"We shouldn't depend on "base".
we can use:
- `partition_alloc/partition_alloc_base/check.h`
- `span_forward_internal.h` -> To be added.
- `partition_alloc/pointers/raw_ptr_exclusion.h`
- `partition_alloc/build_config.h`
Done
#include "base/check.h"
#include "base/compiler_specific.h"
#include "base/containers/checked_iterators.h"
#include "base/containers/span_forward_internal.h"
#include "base/numerics/integral_constant_like.h"
#include "base/numerics/safe_conversions.h"
#include "base/types/to_address.h"Ditto: we can depend on "base". We can only depend on "partition_alloc", so we need to find alternatives to those files.
Agree, I was just in a hurry to share the CL so that i could ask you about the -Wgcc-compat warning.
This already exist as:
base/allocator/partition_allocator/src/partition_alloc/partition_alloc_check.hand `PA_CHECK` macro
Done
Is this file needed somewhere? If not, we can revert adding it.
Done
I believe this file isn't necessary, right?
Done
I believe this file isn't necessary, right?
Done
No need to create this file, because it is already:
"partition_alloc/pointers/raw_ptr_exclusion.h"
Done
I don't believe this file is used, so we can get rid of it?
Done
If we remove `Location`, then this file isn't used anywhere, so it can be removed?
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 4 comments![]( "Open in Gerrit")
CHECK(current_ != start_);`CHECK` => `PA_CHECK`
This is because `CHECK` is from chromium, and `PA_CHECK` from partition_alloc.
DCHECK(other.start_ <= other.current_);`DCHECK` => `PA_DCHECK`
This is because `DCHECK` is from chromium, and `PA_DCHECK` from partition_alloc.
#include "partition_alloc/pointers/raw_ptr_exclusion.h"We can't depend on `raw_ptr_exclusion.h` from `partition_alloc_base`.
This is not required, as the clang pluging doesn't enforce `raw_ptr`.
See:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrManualPathsToIgnore.cpp;l=29
So maybe you can remove this include and the `RAW_PTR_EXCLUSION`?
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef BASE_MACROS_CONCAT_H_
#define BASE_MACROS_CONCAT_H_
// A macro that expands to the concatenation of its arguments. If the arguments
// are themselves macros, they are first expanded (due to the indirection
// through a second macro). This can be used to construct tokens.
#define BASE_CONCAT(a, b) BASE_INTERNAL_CONCAT(a, b)
// Implementation details: do not use directly.
#define BASE_INTERNAL_CONCAT(a, b) a##b
#endif // BASE_MACROS_CONCAT_H_I don't see any usage. Maybe this should be removed?
Related details
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Sergio Solano (Gerrit)
Sergio Solano added 4 comments![]( "Open in Gerrit")
`CHECK` => `PA_CHECK`
This is because `CHECK` is from chromium, and `PA_CHECK` from partition_alloc.
Done
`DCHECK` => `PA_DCHECK`
This is because `DCHECK` is from chromium, and `PA_DCHECK` from partition_alloc.
I missed updating this file. Done now.
#include "partition_alloc/pointers/raw_ptr_exclusion.h"We can't depend on `raw_ptr_exclusion.h` from `partition_alloc_base`.
This is not required, as the clang pluging doesn't enforce `raw_ptr`.
See:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrManualPathsToIgnore.cpp;l=29So maybe you can remove this include and the `RAW_PTR_EXCLUSION`?
Is there a way i sould have known this? It's not clear to me:
1. why we can't depend on `raw_ptr_exclusion.h` from `partition_alloc_base`. Does it has to do with cyclic dependencies?
2. how did you know the clang pluging doesn't enforce `raw_ptr`? By experience or is there some checks I'm missing?
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef BASE_MACROS_CONCAT_H_
#define BASE_MACROS_CONCAT_H_
// A macro that expands to the concatenation of its arguments. If the arguments
// are themselves macros, they are first expanded (due to the indirection
// through a second macro). This can be used to construct tokens.
#define BASE_CONCAT(a, b) BASE_INTERNAL_CONCAT(a, b)
// Implementation details: do not use directly.
#define BASE_INTERNAL_CONCAT(a, b) a##b
#endif // BASE_MACROS_CONCAT_H_Sergio SolanoI don't see any usage. Maybe this should be removed?
Done
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
// Implicit conversion to fixed-extent `std::span<>`. (The fixed-extent
// `std::span` range constructor is explicit.)
// NOLINTNEXTLINE(google-explicit-constructor)
operator std::span<element_type, extent>() const {
return std::span<element_type, extent>(*this);
}
// NOLINTNEXTLINE(google-explicit-constructor)
operator std::span<const element_type, extent>() const
requires(!std::is_const_v<element_type>)
{
return std::span<const element_type, extent>(*this);
}I don't think we will need to support `std::span` conversion for now. Maybe this can be removed?
If we ever need it, we can include it later and update the PRESUBMIT.
Related details
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
#include "partition_alloc/pointers/raw_ptr_exclusion.h"Sergio SolanoWe can't depend on `raw_ptr_exclusion.h` from `partition_alloc_base`.
This is not required, as the clang pluging doesn't enforce `raw_ptr`.
See:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrManualPathsToIgnore.cpp;l=29So maybe you can remove this include and the `RAW_PTR_EXCLUSION`?
Is there a way i sould have known this? It's not clear to me:
1. why we can't depend on `raw_ptr_exclusion.h` from `partition_alloc_base`. Does it has to do with cyclic dependencies?
2. how did you know the clang pluging doesn't enforce `raw_ptr`? By experience or is there some checks I'm missing?
I don't think you could have known. I also spent 10 minutes to think about that before suggesting this solution.
This file is part of "raw_ptr" target. The CQ failure says it isn't a dependency of "allocator_base".
We see "raw_ptr" depends on "allocator_base":
```
gn path ./out/linux-rel base/allocator/partition_allocator/src/partition_alloc:raw_ptr base/allocator/partition_allocator/src/partition_alloc:allocator_base
```
```
//base/allocator/partition_allocator/src/partition_alloc:raw_ptr --[public]-->
//base/allocator/partition_allocator/src/partition_alloc:partition_alloc --[public]-->
//base/allocator/partition_allocator/src/partition_alloc:allocator_base
```
So, it would be a circular dependency for "allocator_base" to depend on "raw_ptr".
We could have added a subtarget for just this file. It would be a dependency of both "raw_ptr" and "allocator_base", but this is a bit ugly.
Technically, "allocator_base" is used to implement "raw_ptr", so we shouldn't really depend on `raw_ptr_exclusion". So I checked how other code in `allocator_base` deal with it. Then I guessed the clang plugin must have an exclusion for avoid this [issue](https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrManualPathsToIgnore.cpp;l=29)
Related details
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 3 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
3 comments
I think I'm even closer to making it. What happens with ERROR A banned pattern was used. `absl::Span and std::span are banned.`? Is there a way to shutdown the error?
#include "partition_alloc/pointers/raw_ptr_exclusion.h"Sergio SolanoWe can't depend on `raw_ptr_exclusion.h` from `partition_alloc_base`.
This is not required, as the clang pluging doesn't enforce `raw_ptr`.
See:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrManualPathsToIgnore.cpp;l=29So maybe you can remove this include and the `RAW_PTR_EXCLUSION`?
Arthur SonzogniIs there a way i sould have known this? It's not clear to me:
1. why we can't depend on `raw_ptr_exclusion.h` from `partition_alloc_base`. Does it has to do with cyclic dependencies?
2. how did you know the clang pluging doesn't enforce `raw_ptr`? By experience or is there some checks I'm missing?
I don't think you could have known. I also spent 10 minutes to think about that before suggesting this solution.
This file is part of "raw_ptr" target. The CQ failure says it isn't a dependency of "allocator_base".
We see "raw_ptr" depends on "allocator_base":
```
gn path ./out/linux-rel base/allocator/partition_allocator/src/partition_alloc:raw_ptr base/allocator/partition_allocator/src/partition_alloc:allocator_base
```
```
//base/allocator/partition_allocator/src/partition_alloc:raw_ptr --[public]-->
//base/allocator/partition_allocator/src/partition_alloc:partition_alloc --[public]-->
//base/allocator/partition_allocator/src/partition_alloc:allocator_base
```So, it would be a circular dependency for "allocator_base" to depend on "raw_ptr".
We could have added a subtarget for just this file. It would be a dependency of both "raw_ptr" and "allocator_base", but this is a bit ugly.
Technically, "allocator_base" is used to implement "raw_ptr", so we shouldn't really depend on `raw_ptr_exclusion". So I checked how other code in `allocator_base` deal with it. Then I guessed the clang plugin must have an exclusion for avoid this [issue](https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrManualPathsToIgnore.cpp;l=29)
Done
// Implicit conversion to fixed-extent `std::span<>`. (The fixed-extent
// `std::span` range constructor is explicit.)
// NOLINTNEXTLINE(google-explicit-constructor)
operator std::span<element_type, extent>() const {
return std::span<element_type, extent>(*this);
}
// NOLINTNEXTLINE(google-explicit-constructor)
operator std::span<const element_type, extent>() const
requires(!std::is_const_v<element_type>)
{
return std::span<const element_type, extent>(*this);
}I don't think we will need to support `std::span` conversion for now. Maybe this can be removed?
If we ever need it, we can include it later and update the PRESUBMIT.
- Arthur Sonzogni
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 8 comments![]( "Open in Gerrit")
There is a lot of includes with partition_alloc equivalent.
Currently span.h is only copied into partition_alloc, further adjustments are needed for it to compile.Could you please format the commit message to fit columns? (Click the format button)
Change-Id: I214b046ab46ab59053b03e8ed7a473a0b238b906
Add:
```
Design doc: go/partition-alloc-spanification
Bug: 481768474
```
"partition_alloc_base/pa_base_export.h",```
PA_COMPONENT_EXPORT(PARTITION_ALLOC_BASE)
```
from
```
#include "partition_alloc/partition_alloc_base/component_export.h"
```
"partition_alloc_base/check_op.cc",
"partition_alloc_base/check_op.h",```
#include "base/allocator/partition_allocator/src/partition_alloc/partition_alloc_check.h"
```
"partition_alloc_base/dcheck_is_on.h",```
#include "base/allocator/partition_allocator/src/partition_alloc/partition_alloc_check.h"
```
"partition_alloc_base/macros/concat.h",`PA_CONCAT` from:
```
#include "base/allocator/partition_allocator/src/partition_alloc/partition_alloc_check.h"
```
"partition_alloc_base/strings/string_view_util.h",I remember we discussed it. It can be removed.
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano added 7 comments![]( "Open in Gerrit")
Currently span.h is only copied into partition_alloc, further adjustments are needed for it to compile.Could you please format the commit message to fit columns? (Click the format button)
Done
Add:
```
Design doc: go/partition-alloc-spanification
Bug: 481768474
```
Done
"partition_alloc_base/pa_base_export.h",Sergio Solano```
PA_COMPONENT_EXPORT(PARTITION_ALLOC_BASE)
```
from
```
#include "partition_alloc/partition_alloc_base/component_export.h"
```
Done
"partition_alloc_base/check_op.cc",
"partition_alloc_base/check_op.h",Sergio Solano```
#include "base/allocator/partition_allocator/src/partition_alloc/partition_alloc_check.h"
```
Done
"partition_alloc_base/dcheck_is_on.h",Sergio Solano```
#include "base/allocator/partition_allocator/src/partition_alloc/partition_alloc_check.h"
```
Done
"partition_alloc_base/macros/concat.h",Sergio Solano`PA_CONCAT` from:
```
#include "base/allocator/partition_allocator/src/partition_alloc/partition_alloc_check.h"
```
Done
I remember we discussed it. It can be removed.
- Arthur Sonzogni
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 14 comments![]( "Open in Gerrit")
"partition_alloc_base/containers/adapters.h",Let's throw away the test "SpanTest, ReverseIterator" and get rid of this dependency?
"partition_alloc_base/macros/is_empty.h",See my other comments. This can probably be removed.
"partition_alloc_base/not_fatal_until.h",Unused? You can remove it.
"partition_alloc_base/numerics/basic_ops_impl.h",If you remove "byte_conversions", you can probably remove this one too.
"partition_alloc_base/numerics/byte_conversions.h",I don't see any usage. I am guessing you reimplementing the missing function with fro span_unittest.cc from "Example_UnsafeBuffersPatterns" using a reinterpret_cast.
This was a good call. Now, can you not add this unused dependency?
"partition_alloc_base/strings/to_string.h",See other comments. This can probably be removed.
"partition_alloc_base/types/is_arc_pointer.h",Why is this added?
"partition_alloc_base/types/supports_ostream_operator.h",Unsure about this one. It might be needed, but I can find reference to where (<<) would be used.
Could you print the error if you stop including it from span_unittest.cc?
#if __has_cpp_attribute(gnu::pure)
#define PURE_FUNCTION [[gnu::pure]]
#else
#define PURE_FUNCTION
#endifUnused? => Remove?
#include "partition_alloc/partition_alloc_base/types/supports_to_string.h"This wasn't part of the original span.h. Do you really need it?
#include "partition_alloc/partition_alloc_base/strings/to_string.h"I don't see any usage. Remove?
#define PARTITION_ALLOC_PARTITION_ALLOC_BASE_IS_EMPTY(...) \Where is this used? Consider removing this file?
// Copyright 2023 The Chromium AuthorsSee my other comment. This can probably be removed.
// Copyright 2023 The Chromium AuthorsSee other comments. This can probably be removed.
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 15 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
15 comments
Let's throw away the test "SpanTest, ReverseIterator" and get rid of this dependency?
Done
See my other comments. This can probably be removed.
Done
Unused? You can remove it.
Done
If you remove "byte_conversions", you can probably remove this one too.
Done
"partition_alloc_base/numerics/byte_conversions.h",I don't see any usage. I am guessing you reimplementing the missing function with fro span_unittest.cc from "Example_UnsafeBuffersPatterns" using a reinterpret_cast.
This was a good call. Now, can you not add this unused dependency?
Done
See other comments. This can probably be removed.
Done
"partition_alloc_base/types/is_arc_pointer.h",Sergio SolanoWhy is this added?
It was a dependency of check_op.h, gone now.
"partition_alloc_base/types/supports_ostream_operator.h",Unsure about this one. It might be needed, but I can find reference to where (<<) would be used.
Could you print the error if you stop including it from span_unittest.cc?
No error.
#if __has_cpp_attribute(gnu::pure)
#define PURE_FUNCTION [[gnu::pure]]
#else
#define PURE_FUNCTION
#endifSergio SolanoUnused? => Remove?
Yes. Do you have a tool that identifies these Unused?
#include "partition_alloc/partition_alloc_base/types/supports_to_string.h"This wasn't part of the original span.h. Do you really need it?
Done
#include "partition_alloc/partition_alloc_base/strings/to_string.h"I don't see any usage. Remove?
Done
#define PARTITION_ALLOC_PARTITION_ALLOC_BASE_IS_EMPTY(...) \Where is this used? Consider removing this file?
Done
See my other comment. This can probably be removed.
Done
See other comments. This can probably be removed.
Done
Related details
- Arthur Sonzogni
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 7 comments![]( "Open in Gerrit")
"partition_alloc_base/types/supports_ostream_operator.h",Sergio SolanoUnsure about this one. It might be needed, but I can find reference to where (<<) would be used.
Could you print the error if you stop including it from span_unittest.cc?
No error.
If there are no errors. Then maybe it can be removd?
// #define PA_ENABLE_IF_ATTR(cond, msg) __attribute__((enable_if(cond, msg)))Should you remove the "//" to enable this?
namespace partition_alloc::internal::base {What do you think about replacing:
```
partition_alloc::internal::base
```
into
```
partition_alloc::base
```
Even if this is expected to be used only from inside partition_alloc, this would avoid having to update the rewritter to emit the "internal" when adding code from within "partition_alloc" namespace instead of "partition_alloc::internal".
Note: this is only for the "span". The other files added can stay in internal.
#include <span>I think we want to remove <span> for now to fix the PRESUBMIT. This is only used to convert a `std::span<T,N>` into a `base::span<T,N>`, but we don't use std::span anyway.
concept IntegralConstantLike =
std::is_integral_v<decltype(T::value)> &&
!std::is_same_v<bool, std::remove_const_t<decltype(T::value)>> &&
std::convertible_to<T, decltype(T::value)> &&
std::equality_comparable_with<T, decltype(T::value)> &&
std::bool_constant<T() == T::value>::value &&
std::bool_constant<static_cast<decltype(T::value)>(T()) == T::value>::value;This is used only once. Maybe it can be inlined in the span.h file?
// Test that base::span can be used with a simple array.I guess this file isn't necessary anymore, because you now have span_unittest ?
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano added 8 comments![]( "Open in Gerrit")
Hi, Arthur, I've made the changes and uploaded my patchset but until now i realize you said that removing <span> would fix the PRESUBMIT. I uploaded my CL with bypass-hooks, I hope that is still accepted.
// #define PA_ENABLE_IF_ATTR(cond, msg) __attribute__((enable_if(cond, msg)))Should you remove the "//" to enable this?
that throws error: ```In file included from ../../base/allocator/partition_allocator/src/partition_alloc/partition_alloc_unittest.cc:37: │
│ ../../base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/containers/span.h:1676:3: error: 'enable_if' attribute cannot be │
│ applied to a statement │
│ 1676 | PA_ENABLE_IF_ATTR(str[Extent - 1u] == CharT{0}, │
│ | ^ │
│ 1677 | "requires string literal as input"); │
│ | ~ │
│ ../../base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/compiler_specific.h:512:53: note: expanded from macro │
│ 'PA_ENABLE_IF_ATTR' │
│ 512 | #define PA_ENABLE_IF_ATTR(cond, msg) __attribute__((enable_if(cond, msg))) │
│ | ^ │
│ 4 errors generated. ```
I have got it to work with changing some functions like: ```
- const CharT (&str PA_LIFETIME_BOUND)[Extent]) {
- PA_ENABLE_IF_ATTR(str[Extent - 1u] == CharT{0},
- "requires string literal as input");
- + const CharT (&str PA_LIFETIME_BOUND)[Extent])
- + PA_ENABLE_IF_ATTR(str[Extent - 1u] == CharT{0},
- + "requires string literal as input") {```
I think it's equivalent but more verbose, is that acceptable?
What do you think about replacing:
```
partition_alloc::internal::base
```
into
```
partition_alloc::base
```Even if this is expected to be used only from inside partition_alloc, this would avoid having to update the rewritter to emit the "internal" when adding code from within "partition_alloc" namespace instead of "partition_alloc::internal".
Note: this is only for the "span". The other files added can stay in internal.
Had to also edit files:
1. span_forward_internal.h
2. checked_iterators.h
3. span_unittest.cc
4. partition_alloc_unittest.cc
What do you think about replacing:
```
partition_alloc::internal::base
```
into
```
partition_alloc::base
```Even if this is expected to be used only from inside partition_alloc, this would avoid having to update the rewritter to emit the "internal" when adding code from within "partition_alloc" namespace instead of "partition_alloc::internal".
Note: this is only for the "span". The other files added can stay in internal.
Done
I think we want to remove <span> for now to fix the PRESUBMIT. This is only used to convert a `std::span<T,N>` into a `base::span<T,N>`, but we don't use std::span anyway.
Done
I think we want to remove <span> for now to fix the PRESUBMIT. This is only used to convert a `std::span<T,N>` into a `base::span<T,N>`, but we don't use std::span anyway.
Done
concept IntegralConstantLike =
std::is_integral_v<decltype(T::value)> &&
!std::is_same_v<bool, std::remove_const_t<decltype(T::value)>> &&
std::convertible_to<T, decltype(T::value)> &&
std::equality_comparable_with<T, decltype(T::value)> &&
std::bool_constant<T() == T::value>::value &&
std::bool_constant<static_cast<decltype(T::value)>(T()) == T::value>::value;This is used only once. Maybe it can be inlined in the span.h file?
Done
// Test that base::span can be used with a simple array.I guess this file isn't necessary anymore, because you now have span_unittest ?
- Arthur Sonzogni
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Hi, Arthur, I've made the changes and uploaded my patchset but until now i realize you said that removing <span> would fix the PRESUBMIT. I uploaded my CL with bypass-hooks, I hope that is still accepted.
There is an other error about using `using namespace`.
Why uploading without checking the presubmit? This would have saved one one round trip no?
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
Arthur SonzogniHi, Arthur, I've made the changes and uploaded my patchset but until now i realize you said that removing <span> would fix the PRESUBMIT. I uploaded my CL with bypass-hooks, I hope that is still accepted.
There is an other error about using `using namespace`.
Why uploading without checking the presubmit? This would have saved one one round trip no?
Yes, I got used to sending this CL with the bypass but now I've fixed the presubmit errors.
Hi Arthur, sorry for bypassing the hooks last time...
Currently I have fixed the presubmit errors:
1. Replaced `if consteval` with `if (std::is_constant_evaluated())` to
satisfy C++20 constraints. (I was getting `PartitionAlloc disallows
C++23 features: `if consteval`).
2. Replaced `std::to_address` with constexpr, `std::pointer_traits` and
`operator->` to avoid presubmit error because the new to_address.h is
not on the `std::to_address` whitelist in PRESUBMIT.py. Would it be
better to update PRESUBMIT.py?
3. Removed `std::span` and `absl::Span` usage and tests to avoid
presubmit error: `A banned pattern was used`.
Related details
- Arthur Sonzogni
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 10 comments![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
10 comments
Thanks for your work!
I think I only have a couple of minor comments below. Please take a look.
LGTM % addressing them.
#endif // BASE_CONTAINERS_CHECKED_ITERATORS_H_Update to match the `#if` / `#ifndef`
// Copyright 2018 The Chromium AuthorsInfo: here is a summary of the differences against the original file.
All of them looks good to me.
### 1. Namespace Changes
- **Moved to PartitionAlloc Namespace:** The entire implementation has been moved from `namespace base` to `namespace partition_alloc::internal::base`.
- **Updated Forward Declarations:** The friend class declaration for `span` was updated from `friend class span;` to the fully qualified `friend class ::partition_alloc::base::span;`.
- **Updated `std::pointer_traits`:** The specialization at the bottom of the file was updated to target `partition_alloc::internal::base::CheckedContiguousIterator<T>` instead of `::base::CheckedContiguousIterator<T>`.
### 2. Include Path Adjustments
- **Path Prefixes:** All internal `#include` paths were rewritten to use the `partition_alloc/` or `partition_alloc/partition_alloc_base/` prefixes instead of `base/` or `build/`.
- **Removed Include:** `#include "base/memory/raw_ptr_exclusion.h"` was entirely removed in the new version.
### 3. Macro Prefixes
Chromium-specific macros were prefixed with `PA_` or `PA_BASE_` to avoid collisions and fit the PartitionAlloc standalone build:
- `CHECK()` `PA_BASE_CHECK()`
- `DCHECK()` `PA_BASE_DCHECK()`
- `UNSAFE_BUFFER_USAGE` `PA_UNSAFE_BUFFER_USAGE`
- `UNSAFE_BUFFERS()` `PA_UNSAFE_BUFFERS()`
### 4. Removal of `RAW_PTR_EXCLUSION`
- The `RAW_PTR_EXCLUSION` macro was stripped from all pointer members.
- In the old version, `start_`, `current_`, and `end_` inside both the `AssumeValid` struct and the private members of the iterator class were annotated with `RAW_PTR_EXCLUSION` (alongside a comment stating "The embedding class is stack-scoped"). In the new version, they are standard raw pointers (e.g., `const T* start_ = nullptr;`).
### 5. Header Guards
- The include guard was updated from `BASE_CONTAINERS_CHECKED_ITERATORS_H_` to `PARTITION_ALLOC_PARTITION_ALLOC_BASE_CONTAINERS_CHECKED_ITERATORS_H_`.
// #ifndef PARTITION_ALLOC_PARTITION_ALLOC_BASE_CONTAINERS_SPAN_H_
// #define PARTITION_ALLOC_PARTITION_ALLOC_BASE_CONTAINERS_SPAN_H_Could you please remove this?
// Copyright 2017 The Chromium AuthorsFor info: here is a summary of the changes:
Here is a comprehensive summary of the differences:
### 1. Namespaces and Include Guards
- **Include Guard:** Changed from `BASE_CONTAINERS_SPAN_H_` to `PARTITION_ALLOC_PARTITION_ALLOC_BASE_CONTAINERS_SPAN_H_`.
- **Namespaces:** Everything is moved out of `namespace base` and into `namespace partition_alloc::base`.
- **Internal Routing:** Uses of `internal::` and base types now explicitly route through `::partition_alloc::internal::base::` (e.g., `bit_cast`, `checked_cast`, `StrictNumeric`, `to_address`).
### 2. Include Path Updates
- All `#include "base/..."` paths have been updated to `#include "partition_alloc/partition_alloc_base/..."`.
- **Header Removals:** `#include <span>` and `#include "base/numerics/integral_constant_like.h"` were removed.
- **Header Additions:** `#include <string_view>` and `#include "partition_alloc/partition_alloc_base/bit_cast.h"` were added.
### 3. Macro Isolation (The `PA_` Prefix)
To avoid colliding with Chromium's core `base` macros, all macros and attributes have been prefixed with `PA_`:
- `CHECK()` / `DCHECK()` -> `PA_BASE_CHECK()` / `PA_BASE_DCHECK()`
- `UNSAFE_BUFFERS()` -> `PA_UNSAFE_BUFFERS()`
- `UNSAFE_BUFFER_USAGE` -> `PA_UNSAFE_BUFFER_USAGE`
- `LIFETIME_BOUND` -> `PA_LIFETIME_BOUND`
- `GSL_POINTER` -> `PA_GSL_POINTER`
- `ENABLE_IF_ATTR` -> `PA_ENABLE_IF_ATTR`
### 4. Downgrade from C++23 to C++20
- **`if consteval` replaced:** The C++23 `if consteval { ... }` blocks (used inside `copy_from` and `copy_from_nonoverlapping`) have been rewritten to use the C++20 equivalent `if (std::is_constant_evaluated()) { ... }`.
### 5. Standard Library Decoupling
- **Removal of `std::span`:** All interoperability with `std::span` has been stripped out. The explicit/implicit conversion operators (`operator std::span<element_type, extent>() const`) were completely removed, as was the specific `kComputedExtentImpl` deduction guide for `std::span`.
- **Removal of `std::make_unique`:** Inside the `constexpr` blocks for `copy_from`, the old code used `auto buffer = std::make_unique<Holder[]>(extent);`. Because `std::unique_ptr` isn't fully constexpr-friendly prior to C++23 without `<vector>`, the new code falls back to raw pointer allocation: `Holder* buffer = new Holder[extent]; ... delete[] buffer;`.
### 6. Compiler Pragmas added
- The new file wraps the entire header in Clang diagnostic pragmas to suppress `-Wgcc-compat` warnings:
- ```cpp
- #if defined(__clang__)
- #pragma clang diagnostic push
- #pragma clang diagnostic ignored "-Wgcc-compat"
- #endif
- // ... file content ...
- #pragma clang diagnostic pop
```
### 7. New Functionality
- **`as_string_view` helpers:** The new code introduces several overloads for `as_string_view()` at the very bottom of the file to easily convert byte-like spans (`span<const char>`, `span<const unsigned char>`, `span<const char16_t>`, etc.) into their respective `std::string_view` types.
#endif // BASE_CONTAINERS_SPAN_FORWARD_INTERNAL_H_Update to match the `#if` / `#ifndef`
// Alas, many desirable comparisions are still not possible. TheyPlease fix this WARNING reported by Spellchecker: "comparisions" is a possible misspelling of "comparisons".
To bypass Spellcheck...
"comparisions" is a possible misspelling of "comparisons".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
#endif // BASE_STRINGS_UTF_OSTREAM_OPERATORS_H_Update to match the `#if` / `#ifndef`
#endif // BASE_TYPES_TO_ADDRESS_H_Update to match the `#if` / `#ifndef`
}
} // namespace partition_alloc::internalOptional: Since it isn't meaningful to update this file, you can revert this part?
```
git checkout HEAD~ -- base/allocator/partition_allocator/src/partition_alloc/partition_alloc_unittest.cc
```
Related details
- Arthur Sonzogni
- Sergio Solano
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano added 7 comments![]( "Open in Gerrit")
Update to match the `#if` / `#ifndef`
Done
// #ifndef PARTITION_ALLOC_PARTITION_ALLOC_BASE_CONTAINERS_SPAN_H_
// #define PARTITION_ALLOC_PARTITION_ALLOC_BASE_CONTAINERS_SPAN_H_Could you please remove this?
Done
Update to match the `#if` / `#ifndef`
Done
// Alas, many desirable comparisions are still not possible. TheyPlease fix this WARNING reported by Spellchecker: "comparisions" is a possible misspelling of "comparisons".
To bypass Spellcheck...
"comparisions" is a possible misspelling of "comparisons".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
Update to match the `#if` / `#ifndef`
Done
Update to match the `#if` / `#ifndef`
Done
Optional: Since it isn't meaningful to update this file, you can revert this part?
```
git checkout HEAD~ -- base/allocator/partition_allocator/src/partition_alloc/partition_alloc_unittest.cc
```
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 2 comments![]( "Open in Gerrit")
This is looking pretty good, going to pull it down and verify the diffs as Arthur has noted in his comments and then this will LGTM, just leaving the one comment about utf_ostreams which I think we might be able to remove?
#include "partition_alloc/partition_alloc_base/strings/utf_ostream_operators.h"Can we remove this dependency?
I don't even see where the ostream is in this file?
Bonus we don't need to bring it in, which I'd like, can you tell me the lines that are using this and I can think if there are reasonable alternatives?
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
1 comment
#include "partition_alloc/partition_alloc_base/strings/utf_ostream_operators.h"Can we remove this dependency?
I don't even see where the ostream is in this file?
Bonus we don't need to bring it in, which I'd like, can you tell me the lines that are using this and I can think if there are reasonable alternatives?
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 20 comments![]( "Open in Gerrit")
"partition_alloc_base/strings/utf_ostream_operators.h",This file can be removed now.
Info: here is a summary of the differences against the original file.
All of them looks good to me.
### 1. Namespace Changes
- **Moved to PartitionAlloc Namespace:** The entire implementation has been moved from `namespace base` to `namespace partition_alloc::internal::base`.
- **Updated Forward Declarations:** The friend class declaration for `span` was updated from `friend class span;` to the fully qualified `friend class ::partition_alloc::base::span;`.
- **Updated `std::pointer_traits`:** The specialization at the bottom of the file was updated to target `partition_alloc::internal::base::CheckedContiguousIterator<T>` instead of `::base::CheckedContiguousIterator<T>`.
### 2. Include Path Adjustments
- **Path Prefixes:** All internal `#include` paths were rewritten to use the `partition_alloc/` or `partition_alloc/partition_alloc_base/` prefixes instead of `base/` or `build/`.
- **Removed Include:** `#include "base/memory/raw_ptr_exclusion.h"` was entirely removed in the new version.
### 3. Macro Prefixes
Chromium-specific macros were prefixed with `PA_` or `PA_BASE_` to avoid collisions and fit the PartitionAlloc standalone build:
- `CHECK()` `PA_BASE_CHECK()`
- `DCHECK()` `PA_BASE_DCHECK()`
- `UNSAFE_BUFFER_USAGE` `PA_UNSAFE_BUFFER_USAGE`
- `UNSAFE_BUFFERS()` `PA_UNSAFE_BUFFERS()`
### 4. Removal of `RAW_PTR_EXCLUSION`
- The `RAW_PTR_EXCLUSION` macro was stripped from all pointer members.
- In the old version, `start_`, `current_`, and `end_` inside both the `AssumeValid` struct and the private members of the iterator class were annotated with `RAW_PTR_EXCLUSION` (alongside a comment stating "The embedding class is stack-scoped"). In the new version, they are standard raw pointers (e.g., `const T* start_ = nullptr;`).
### 5. Header Guards
- The include guard was updated from `BASE_CONTAINERS_CHECKED_ITERATORS_H_` to `PARTITION_ALLOC_PARTITION_ALLOC_BASE_CONTAINERS_CHECKED_ITERATORS_H_`.
Verified all these changes locally with diff.
if (std::is_constant_evaluated()) {nit: Add a `TODO(c++23): Switch to if consteval once PA supports only c++23 and above.`
std::construct_at(&PA_UNSAFE_BUFFERS(buffer[i]).value, other[i]);same optional nit:
```suggestion
std::construct_at(PA_UNSAFE_BUFFERS(&buffer[i]).value, other[i]);
```
if (std::is_constant_evaluated()) {nit: Add a `TODO(c++23): Switch to if consteval once PA supports only c++23 and above.`
if (std::is_constant_evaluated()) {nit: Add a `TODO(c++23): Switch to if consteval once PA supports only c++23 and above.`
std::construct_at(&PA_UNSAFE_BUFFERS(buffer[i]).value, other[i]);optional nit: I find it strange to see the '&' outside the UNSAFE_BUFFERS
```suggestion
std::construct_at(PA_UNSAFE_BUFFERS(&buffer[i]).value, other[i]);
```
if (std::is_constant_evaluated()) {nit: Add a `TODO(c++23): Switch to if consteval once PA supports only c++23 and above.`
using ::partition_alloc::internal::base::bit_cast;
using ::partition_alloc::internal::base::checked_cast;
using ::partition_alloc::internal::base::CheckedContiguousConstIterator;
using ::partition_alloc::internal::base::CheckedContiguousIterator;
using ::partition_alloc::internal::base::StrictNumeric;
using ::partition_alloc::internal::base::to_address;This has the effect of making these internal functions become part of `partition_alloc::base` namespace (anyone can refer to them as such if they include span.h).
Is there a reason to have them be in the internal namespace to begin with?
// Copyright 2017 The Chromium AuthorsThanks, I've verified the all of these by diffing locally.
Question about 5) Is this just because folks that depend on PA might not be compiling with a full std::span implementation (assuming Angle and downstream of them)? Or what was the reasling for removing `std::span` otherwise?
The test `ConstructFromDataAndSize` was deleted do you know why?
Why was the AsWritableBytes and AsChars test deleted AsWritableChars?
Presuming that abslHash got deleted because PA doesn't use absl, let me know if I'm wrong.
The safe alternative was deleted, is this because we don't have `U32FromLittleEndian`?
Could we just do something like.
`uint64_t v2 = *(reinterpret_cast<const uint64_t*>(&span(array)[6u]));`?
it was also removed from the one below.
note: std tests were removed because std support was removed.
// Use of this source code is governed by a BSD-style license that can beThis file can be removed now
if constexpr (requires { std::pointer_traits<P>::to_address(p); }) {
return std::pointer_traits<P>::to_address(p);
} else {
return p.operator->();
}This changed versus the base version which just uses `std::to_address`, which is a c++20 type, do you know what caused this constexpr addition here?
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Thanks Stephen.
For now, PartitionAlloc is subject to the Chromium requirements around std::span. So this would trigger a PRESUBMIT.py.
We might need to interact with libraries that are interacting with span at some point in the future, but I strongly suspect we don't, and will never have to.
So, instead of asking a PRESUBMIT.py exception for PartitionAlloc, I said can ignore this for now and strip out the `std::span` logic for now. Then we can merge this quickly.
If we need it in the future, we can reconsider bringing this back.
Related details
- Sergio Solano
Sergio Solano (Gerrit)
Sergio Solano voted and added 17 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
17 comments
"partition_alloc_base/strings/utf_ostream_operators.h",This file can be removed now.
Done
if (std::is_constant_evaluated()) {nit: Add a `TODO(c++23): Switch to if consteval once PA supports only c++23 and above.`
Done
std::construct_at(&PA_UNSAFE_BUFFERS(buffer[i]).value, other[i]);same optional nit:
```suggestion
std::construct_at(PA_UNSAFE_BUFFERS(&buffer[i]).value, other[i]);
```
Done
if (std::is_constant_evaluated()) {nit: Add a `TODO(c++23): Switch to if consteval once PA supports only c++23 and above.`
Done
if (std::is_constant_evaluated()) {nit: Add a `TODO(c++23): Switch to if consteval once PA supports only c++23 and above.`
Done
std::construct_at(&PA_UNSAFE_BUFFERS(buffer[i]).value, other[i]);optional nit: I find it strange to see the '&' outside the UNSAFE_BUFFERS
```suggestion
std::construct_at(PA_UNSAFE_BUFFERS(&buffer[i]).value, other[i]);
```
Done
if (std::is_constant_evaluated()) {nit: Add a `TODO(c++23): Switch to if consteval once PA supports only c++23 and above.`
Done
using ::partition_alloc::internal::base::bit_cast;
using ::partition_alloc::internal::base::checked_cast;
using ::partition_alloc::internal::base::CheckedContiguousConstIterator;
using ::partition_alloc::internal::base::CheckedContiguousIterator;
using ::partition_alloc::internal::base::StrictNumeric;
using ::partition_alloc::internal::base::to_address;This has the effect of making these internal functions become part of `partition_alloc::base` namespace (anyone can refer to them as such if they include span.h).
Is there a reason to have them be in the internal namespace to begin with?
Added alias `namespace pa_base = ::partition_alloc::internal::base;` inside internal to be less verbose when using StrictNumeric without making these external functions part of the public PA base namespace.
The test `ConstructFromDataAndSize` was deleted do you know why?
I've added `ConstructFromDataAndSize` except the part that uses `fixed_extent`.
Currently, `strict_cast` in PartitionAlloc's base only handles arithmetic types. To support the full test, we would need to update `StrictNumeric` and `strict_cast` to handle `std::integral_constant` (which `fixed_extent` uses), similar to the main //base implementation. I don't think the gains of having the complete test justify the effort. What do you think?
Why was the AsWritableBytes and AsChars test deleted AsWritableChars?
Added the tests without the raw_ptr y raw_span parts.
Was was Indexing, CopyFrom, deleted?
Marked as resolved.
Why was SplitAt TakeFirst deleted?
Done
The safe alternative was deleted, is this because we don't have `U32FromLittleEndian`?
Could we just do something like.
`uint64_t v2 = *(reinterpret_cast<const uint64_t*>(&span(array)[6u]));`?
it was also removed from the one below.
Done
// Use of this source code is governed by a BSD-style license that can beThis file can be removed now
Done
if constexpr (requires { std::pointer_traits<P>::to_address(p); }) {
return std::pointer_traits<P>::to_address(p);
} else {
return p.operator->();
}This changed versus the base version which just uses `std::to_address`, which is a c++20 type, do you know what caused this constexpr addition here?
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 8 comments![]( "Open in Gerrit")
: data_(::partition_alloc::base::to_address(first)) {aren't we already in ::partition_alloc::base? wouldn't just `to_address` work? here and below.
internal::pa_base::StrictNumeric<size_type> count)why not just `internal::base::StrictNumeric<size_type>` isn't `pa_base` +2 letters?
Or did you want this to be
`pa_base::StrictNumeric<size_type>`?
In which case you should put this
`using pa_base = ::partition_alloc::internal::base;` inside the span class perhaps and use that as `pa_base::StrictNumeric<size_type>`?
using ::partition_alloc::base::to_address;What is this using statement for?
Is internal enough in this case?
// Copyright 2017 The Chromium AuthorsYeah perfectly reasonable just wanted to know for my own knowledge.
// Storage pointer customization. By default this is not a
// `raw_ptr<>`, since `span` is mostly used for stack variables. Use
// `raw_span` instead for class fields, which sets this to
// `raw_ptr<T>`.Can we even use raw_ptr inside partition_alloc? It is probably to self referential? And perhaps we remove this comment?
{
const std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.cend())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.end())), span<const int>>);
}
{
std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.cend())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.end())), span<int>>);
}
// Tests for span(Range&&) deduction guide.
// C-style arrays.
static_assert(std::is_same_v<decltype(span(std::declval<const int (&)[3]>())),
span<const int, 3>>);
static_assert(
std::is_same_v<decltype(span(std::declval<const int (&&)[3]>())),
span<const int, 3>>);
static_assert(
std::is_same_v<decltype(span(std::declval<int (&)[3]>())), span<int, 3>>);
static_assert(std::is_same_v<decltype(span(std::declval<int (&&)[3]>())),
span<const int, 3>>);
// std::array<const T, N>.
static_assert(
std::is_same_v<decltype(span(
std::declval<const std::array<const bool, 3>&>())),
span<const bool, 3>>);
static_assert(
std::is_same_v<decltype(span(
std::declval<const std::array<const bool, 3>&&>())),
span<const bool, 3>>);
static_assert(
std::is_same_v<decltype(span(std::declval<std::array<const bool, 3>&>())),
span<const bool, 3>>);
static_assert(std::is_same_v<
decltype(span(std::declval<std::array<const bool, 3>&&>())),
span<const bool, 3>>);
// std::array<T, N>.
static_assert(
std::is_same_v<decltype(span(std::declval<const std::array<bool, 3>&>())),
span<const bool, 3>>);
static_assert(std::is_same_v<
decltype(span(std::declval<const std::array<bool, 3>&&>())),
span<const bool, 3>>);
static_assert(
std::is_same_v<decltype(span(std::declval<std::array<bool, 3>&>())),
span<bool, 3>>);
static_assert(
std::is_same_v<decltype(span(std::declval<std::array<bool, 3>&&>())),
span<const bool, 3>>);
// std::string.
static_assert(
std::is_same_v<decltype(span(std::declval<const std::string&>())),
span<const char>>);
static_assert(
std::is_same_v<decltype(span(std::declval<const std::string&&>())),
span<const char>>);
static_assert(
std::is_same_v<decltype(span(std::declval<std::string&>())), span<char>>);
static_assert(std::is_same_v<decltype(span(std::declval<std::string&&>())),
span<const char>>);
// std::u16string.
static_assert(
std::is_same_v<decltype(span(std::declval<const std::u16string&>())),
span<const char16_t>>);
static_assert(
std::is_same_v<decltype(span(std::declval<const std::u16string&&>())),
span<const char16_t>>);
static_assert(std::is_same_v<decltype(span(std::declval<std::u16string&>())),
span<char16_t>>);
static_assert(std::is_same_v<decltype(span(std::declval<std::u16string&&>())),
span<const char16_t>>);
// std::ranges::subrange<const T*>.
static_assert(std::is_same_v<
decltype(span(
std::declval<const std::ranges::subrange<const int*>&>())),
span<const int>>);
static_assert(std::is_same_v<
decltype(span(
std::declval<const std::ranges::subrange<const int*>&&>())),
span<const int>>);
static_assert(
std::is_same_v<decltype(span(
std::declval<std::ranges::subrange<const int*>&>())),
span<const int>>);
static_assert(
std::is_same_v<decltype(span(
std::declval<std::ranges::subrange<const int*>&&>())),
span<const int>>);
// std::ranges::subrange<T*>.
static_assert(
std::is_same_v<decltype(span(
std::declval<const std::ranges::subrange<int*>&>())),
span<int>>);
static_assert(
std::is_same_v<decltype(span(
std::declval<const std::ranges::subrange<int*>&&>())),
span<int>>);
static_assert(std::is_same_v<
decltype(span(std::declval<std::ranges::subrange<int*>&>())),
span<int>>);
static_assert(std::is_same_v<
decltype(span(std::declval<std::ranges::subrange<int*>&&>())),
span<int>>);There are suddenly a bunch of deletions was this intentional (here and elsewhere). It makes it hard to check the added tests and also raises the question of why they were removed (were they failing?)
Sergio SolanoThe test `ConstructFromDataAndSize` was deleted do you know why?
I've added `ConstructFromDataAndSize` except the part that uses `fixed_extent`.
Currently, `strict_cast` in PartitionAlloc's base only handles arithmetic types. To support the full test, we would need to update `StrictNumeric` and `strict_cast` to handle `std::integral_constant` (which `fixed_extent` uses), similar to the main //base implementation. I don't think the gains of having the complete test justify the effort. What do you think?
This seems like a reasonable compromise.
if constexpr (requires { std::pointer_traits<P>::to_address(p); }) {
return std::pointer_traits<P>::to_address(p);
} else {
return p.operator->();
}Sergio SolanoThis changed versus the base version which just uses `std::to_address`, which is a c++20 type, do you know what caused this constexpr addition here?
Fixed. Don't remember the reason for changing.
This implementation is still quite different from base, do we not need the SFINAE wrapper? Can you add some comments if that is the case in which case do we need this function at all?
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Sergio, make you can add a top-level comment?
```
// This is a simplified version of Chromium's
// //base/containers/span_unittest.cc. To minimize dependencies, several tests
// and APIs were removed:
//
// 1. std::span Interoperability (FromStdSpan, ToStdSpan)
// Rationale: Avoids triggering Chromium PRESUBMIT.py checks for `std::span`.
// We don't currently need this interoperability. We can revisit this if
// needed in the future.
//
// 2. Type Punning (ReinterpretSpan)
// Rationale: This was introduced in upstream //base after our initial
// migration.
// TODO(sergiosolano): Should we port this over and add it back in?
//
// 3. Abseil Dependencies (AbslHash)
// Rationale: PartitionAlloc doesn't depend on Abseil.
//
// 4. Volatile Memory Support (ConstructFromVolatileArray, CopyFromVolatile)
// TODO(sergiosolano): Please clarify why these were removed. I am not sure
// to remember.
//
// 5. String/C-String Utilities (FromCString, FromCStringEmpty, etc.)
// Rationale: PA strictly handles byte-level memory, so text-focused
// utilities and their tests are unnecessary. This avoids importing
// base::cstring_view and its dependencies, which are not likely not needed.
//
// 6. Exhaustive API Testing (Slicing, Iterators, Comparisons, Conversions)
// TODO(sergiosolano): Please document the exact rationale for removing
// these test cases. I don't remember why.
//
// 7. Death Tests and Macros (OutOfBoundsDeath, GTest/GMock Compatibility)
// Rationale: Death tests rely on Chromium-specific build config macros.
// Removing them avoids pulling in those external dependencies just to
// make the tests run reliably.
// -----------------------------------------------------------------------------
```
Related details
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 7 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
7 comments
Hi, guys. I hit a presubmit error with std::to_address because it's banned globally. To keep to_address.h as a copy of //base/types/to_address.h (which also uses std::to_address in its implementation), I've added our new path to the exception list in the root PRESUBMIT.py. This is consistent with how //base handles its own wrapper.
Do you agree?
: data_(::partition_alloc::base::to_address(first)) {aren't we already in ::partition_alloc::base? wouldn't just `to_address` work? here and below.
Done
why not just `internal::base::StrictNumeric<size_type>` isn't `pa_base` +2 letters?
Or did you want this to be
`pa_base::StrictNumeric<size_type>`?
In which case you should put this
`using pa_base = ::partition_alloc::internal::base;` inside the span class perhaps and use that as `pa_base::StrictNumeric<size_type>`?
I've brought StrictNumeric directly into the partition_alloc::base::internal namespace. This allows using internal::StrictNumeric at the call sites without the need for an extra alias layer or placing it inside the class.
What is this using statement for?
Is internal enough in this case?
I've removed that using statement.
// Storage pointer customization. By default this is not a
// `raw_ptr<>`, since `span` is mostly used for stack variables. Use
// `raw_span` instead for class fields, which sets this to
// `raw_ptr<T>`.Can we even use raw_ptr inside partition_alloc? It is probably to self referential? And perhaps we remove this comment?
Comment removed.
Done
if constexpr (requires { std::pointer_traits<P>::to_address(p); }) {
return std::pointer_traits<P>::to_address(p);
} else {
return p.operator->();
}Sergio SolanoThis changed versus the base version which just uses `std::to_address`, which is a c++20 type, do you know what caused this constexpr addition here?
Stephen NuskoFixed. Don't remember the reason for changing.
This implementation is still quite different from base, do we not need the SFINAE wrapper? Can you add some comments if that is the case in which case do we need this function at all?
I'm not really sure if PartitionAlloc needs this. I am changing the file to be as similar to the original.
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 2 comments![]( "Open in Gerrit")
Will do a side by side diff from base again tomorrow, but looking reasonable I think.
// Rationale: Death tests rely on Chromium-specific build config macros.
// Removing them avoids pulling in those external dependencies just to
// make the tests run reliably.PartitionAlloc has `PA_USE_DEATH_TESTS`, I'll take a look but could you expand one what sort of BUILD config macros?
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 6 comments![]( "Open in Gerrit")
Do you mind double checking the logic for all the removals in the tests? Seems like a lot got deleted that could be fixed, we won't ever import span again and we'd like to make sure our implementation stays well tested at least.
namespace partition_alloc::base::internal {
using ::partition_alloc::internal::base::StrictNumeric;
} // namespace partition_alloc::base::internal
namespace partition_alloc::base {
You can move this down into the namespace below.
```suggestion
using ::partition_alloc::internal::base::StrictNumeric;
} // namespace partition_alloc::base::internal
namespace partition_alloc::base {
namespace internal {
using ::partition_alloc::internal::base::StrictNumeric;
} // namespace internal
```
But also I have some questions
why do we have a `partition_alloc::base` and a `partition_alloc::internal::base` to begin with?
I can't see any previous use-case of `partition_alloc::base` is this the first introduction of it in this CL?
I remember there was discussions of namespaces but this seems to be a bit conflicted because if this should be in base::internal StrictNumeric should declare itself there.
See for example how we redeclare things in [PartitionLock](https://source.chromium.org/chromium/chromium/src/+/main:base/allocator/partition_allocator/src/partition_alloc/partition_lock.h;l=181-188;drc=e72a72ba9fe98e14b9d255722b7704ae84f5b8df), but should we just remove partition_alloc::internal for partition_alloc::base::internal?
// Rationale: Death tests rely on Chromium-specific build config macros.
// Removing them avoids pulling in those external dependencies just to
// make the tests run reliably.PartitionAlloc has `PA_USE_DEATH_TESTS`, I'll take a look but could you expand one what sort of BUILD config macros?
For OutOfBoundsDeath test, could you just do
```
#if PA_USE_DEATH_TESTS
TEST(PASpanTest, OutOfBoundsDeath) {
constexpr span<int, 0> kEmptySpan;
EXPECT_DEATH({kEmptySpan.first(1u);}, "");
}
#endif // PA_USE_DEATH_TESTS
```
?
// Tests for span(It, EndOrSize) deduction guide.
{
const std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.data(), v.size())), span<const int>>);
}
{
std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.size())), span<int>>);
static_assert(
std::is_same_v<decltype(span(v.data(), v.size())), span<int>>);
}
}This test was greatly reduced, but it only used standard std types rational given in number 6 doesn't apply. For example:
I.E. all of these seem trival to have in partitionAlloc as well: https://source.chromium.org/chromium/chromium/src/+/main:base/containers/span_unittest.cc;l=85-184;drc=9a040b86879feaa0d33e564776741f4729e5c4d1
ConstructFromDataAndSize again doesn't seem to fall into number 6, I don't see why we couldn't keep it?
ConstructFromDataAndZeroSize has a DEATH check but if you use
```
#if PA_USE_DEATH_TESTS
PA_EXPECT_DCHECK_DEATH(({ PA_UNSAFE_BUFFERS(span<char>(nullptr_to_char, 123u)); });
#endif
```
std::vector<int> vector = {1, 1, 2, 3, 5, 8};This removed the construct from empty
```
constexpr int* kNull = nullptr;
constexpr span<int> UNSAFE_BUFFERS(empty_span(kNull, kNull));
EXPECT_TRUE(empty_span.empty());
EXPECT_EQ(nullptr, empty_span.data());
```
Do you mind going through this and explaining a bit clearer why you removed tests you did? This one seems like it would work just fine?
Sergio Solano (Gerrit)
Sergio Solano added 6 comments![]( "Open in Gerrit")
Hi, thanks for the reviews. Here is my next attempt.
namespace partition_alloc::base::internal {
using ::partition_alloc::internal::base::StrictNumeric;
} // namespace partition_alloc::base::internal
namespace partition_alloc::base {
You can move this down into the namespace below.
```suggestion
using ::partition_alloc::internal::base::StrictNumeric;
} // namespace partition_alloc::base::internalnamespace partition_alloc::base {
namespace internal {
using ::partition_alloc::internal::base::StrictNumeric;
} // namespace internal
```But also I have some questions
why do we have a `partition_alloc::base` and a `partition_alloc::internal::base` to begin with?
I can't see any previous use-case of `partition_alloc::base` is this the first introduction of it in this CL?
I remember there was discussions of namespaces but this seems to be a bit conflicted because if this should be in base::internal StrictNumeric should declare itself there.
See for example how we redeclare things in [PartitionLock](https://source.chromium.org/chromium/chromium/src/+/main:base/allocator/partition_allocator/src/partition_alloc/partition_lock.h;l=181-188;drc=e72a72ba9fe98e14b9d255722b7704ae84f5b8df), but should we just remove partition_alloc::internal for partition_alloc::base::internal?
Sorry, I introduced partition_alloc::base by mistake in an earlier patch. I hope my understanding of namespaces is better now. I've changed the CL to the pre-existing partition_alloc::internal::base namespace. Now there is no need for StrictNumeric aliases.
// Rationale: Death tests rely on Chromium-specific build config macros.
// Removing them avoids pulling in those external dependencies just to
// make the tests run reliably.Stephen NuskoPartitionAlloc has `PA_USE_DEATH_TESTS`, I'll take a look but could you expand one what sort of BUILD config macros?
For OutOfBoundsDeath test, could you just do
```
#if PA_USE_DEATH_TESTS
TEST(PASpanTest, OutOfBoundsDeath) {
constexpr span<int, 0> kEmptySpan;
EXPECT_DEATH({kEmptySpan.first(1u);}, "");
}
#endif // PA_USE_DEATH_TESTS
```?
Done
// Tests for span(It, EndOrSize) deduction guide.
{
const std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.data(), v.size())), span<const int>>);
}
{
std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.size())), span<int>>);
static_assert(
std::is_same_v<decltype(span(v.data(), v.size())), span<int>>);
}
}This test was greatly reduced, but it only used standard std types rational given in number 6 doesn't apply. For example:
I.E. all of these seem trival to have in partitionAlloc as well: https://source.chromium.org/chromium/chromium/src/+/main:base/containers/span_unittest.cc;l=85-184;drc=9a040b86879feaa0d33e564776741f4729e5c4d1
I've re-reviewed the original base/containers/span_unittest.cc and realized that my simplification went too far. I have restored all the tests that required no new dependencies (I've still omitted the once that require base::cstring_view or Abseil). I've also updated the comment at the top of the file to reflect the set of omissions.
ConstructFromDataAndSize again doesn't seem to fall into number 6, I don't see why we couldn't keep it?
ConstructFromDataAndZeroSize has a DEATH check but if you use
```
#if PA_USE_DEATH_TESTS
PA_EXPECT_DCHECK_DEATH(({ PA_UNSAFE_BUFFERS(span<char>(nullptr_to_char, 123u)); });
#endif
```
Done
This removed the construct from empty
```
constexpr int* kNull = nullptr;
constexpr span<int> UNSAFE_BUFFERS(empty_span(kNull, kNull));
EXPECT_TRUE(empty_span.empty());
EXPECT_EQ(nullptr, empty_span.data());
```Do you mind going through this and explaining a bit clearer why you removed tests you did? This one seems like it would work just fine?
Sorry, I did some unnecessary omissions, hope the current state is better. Thanks for the review, it's helping me understand a bunch.
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 4 comments![]( "Open in Gerrit")
m
// Tests for span(It, EndOrSize) deduction guide.
{
const std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.data(), v.size())), span<const int>>);
}
{
std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.size())), span<int>>);
static_assert(
std::is_same_v<decltype(span(v.data(), v.size())), span<int>>);
}
}Sergio SolanoThis test was greatly reduced, but it only used standard std types rational given in number 6 doesn't apply. For example:
I.E. all of these seem trival to have in partitionAlloc as well: https://source.chromium.org/chromium/chromium/src/+/main:base/containers/span_unittest.cc;l=85-184;drc=9a040b86879feaa0d33e564776741f4729e5c4d1
I've re-reviewed the original base/containers/span_unittest.cc and realized that my simplification went too far. I have restored all the tests that required no new dependencies (I've still omitted the once that require base::cstring_view or Abseil). I've also updated the comment at the top of the file to reflect the set of omissions.
Thanks!
Can you look at:
* `NoLifetimeWarnings` test
* I think it should be able to be included? It was sandwitched inbetween the CString tests, but itself doesn't actually reference them.
* `Subspan` tests, looks like a bunch of cases were deleted.
* The matchers are just standard GTEST matches they should be able to be included. (ElementsAre & IsEmpty).
* `Size` test
* it was edited to remove the {} scopes which makes the names have to be unique, we can leave it as it was in the original to reduce churn.
* `Empty` test
* the span of end iterators is an empty span seems like a valid test which got deleted.
* `AsWriteableByteSpan` test
* the little endian test was deleted, it should continue to work? `EXPECT_EQ(byte_span[0u], 2);`
* `OutOfBoundsDeath` test
* stl algorithms should be fine to test as well right? So even though we don't support std::span we can still test std::copy, std::copy_n, etc I think?
* `CopyFromNonoverlapping`
* Looks like we removed some expectations that should work (ElementsAre is a standard GTEST matcher). The span_from_cstring can probably be replaced with an std::string_view or just a span<char> to do the same test.
* `CopyFromConversion`
* again looks like some tests were deleted that I don't understand the reason for (ElementsAre is a standard GTEST matcher).
* `CompareEquality`, `CompareOrdered`, `GMockMacroCompatibility`, and`GTestMacroCompatability` tests
* seems to have completely been removed but seems valid overall?
std::vector<int> vector = {1, 1, 2, 3, 5, 8};Sergio SolanoThis removed the construct from empty
```
constexpr int* kNull = nullptr;
constexpr span<int> UNSAFE_BUFFERS(empty_span(kNull, kNull));
EXPECT_TRUE(empty_span.empty());
EXPECT_EQ(nullptr, empty_span.data());
```Do you mind going through this and explaining a bit clearer why you removed tests you did? This one seems like it would work just fine?
Sorry, I did some unnecessary omissions, hope the current state is better. Thanks for the review, it's helping me understand a bunch.
No worries, it is a large CL with a lot of moving parts 😊 Easy to delete things.
(void)val_span;What is this `(void)val_span;` doesn't seem to be in bases copy and I believe it doesn't do anything?
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano added 3 comments![]( "Open in Gerrit")
Hi, guys, new iteration. I hope this time the code is good enough.
// Tests for span(It, EndOrSize) deduction guide.
{
const std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.data(), v.size())), span<const int>>);
}
{
std::vector<int> v;
static_assert(
std::is_same_v<decltype(span(v.cbegin(), v.size())), span<const int>>);
static_assert(
std::is_same_v<decltype(span(v.begin(), v.size())), span<int>>);
static_assert(
std::is_same_v<decltype(span(v.data(), v.size())), span<int>>);
}
}I have added all missing tests and parts you mention.
About `span_from_cstring`, I added the test as is in base/containers/span_unittest.cc. `span_from_cstring` is defined in partition_alloc's span.h and all tests pass. Is it ok to have span_from_cstring in span.h or does it contradicts the idea of staying away from string utilities?
What is this `(void)val_span;` doesn't seem to be in bases copy and I believe it doesn't do anything?
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 4 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
4 comments
Thanks for keeping at it. I think this looks reasonable to me.
If it is defined in span.h I think it seems reasonable to test it, and we probably should.
If however you are asking should we remove this support completely because we don't suspect we'll do string manipulation... Maybe? I'm unsure what we will and what we won't use in this regards. however supporting a conversion from a string_view to a span does not itself really cross into a "string" utility so it would have to live in this file I think if we want it.
Feel free to remove as per reason (5) at the top of the file.
static_assert(std::is_same_v<decltype(byte_span_from_ref(std::move(x))),
span<const uint8_t, sizeof(int)>>);
EXPECT_EQ(reinterpret_cast<const uint8_t*>(&x),
byte_span_from_ref(std::move(x)).data());This was added versus, base. Doesn't seem terrible, but also did you have a reason to add it? Seems like an additional test of as_bytes?
partition_alloc::internal::base::debug::Alias(&fixed_span[3u]));nit: we are already in `partition_alloc::internal::base` so this should only need `debug::Alias`. Please check other locations as well.
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano added 3 comments![]( "Open in Gerrit")
Hi, guys, I've fixed the (hopefully) last two comments.
static_assert(std::is_same_v<decltype(byte_span_from_ref(std::move(x))),
span<const uint8_t, sizeof(int)>>);
EXPECT_EQ(reinterpret_cast<const uint8_t*>(&x),
byte_span_from_ref(std::move(x)).data());This was added versus, base. Doesn't seem terrible, but also did you have a reason to add it? Seems like an additional test of as_bytes?
Now I don't see a reason to add it since there is TEST AsBytes. I'll delete it. Sorry for the unjustified diffs between this and base.
partition_alloc::internal::base::debug::Alias(&fixed_span[3u]));nit: we are already in `partition_alloc::internal::base` so this should only need `debug::Alias`. Please check other locations as well.
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 7 comments![]( "Open in Gerrit")
Excellent!
// allocation (it is not the pointer toone-past-the-end).nit: "to one"
// `array` has three elmenents, so `span` has three elements, sotypo
// `array` has three elmenents, so `span` has three elements, sotypo
// `array` has three elmenents, so `span` has three elements, sotypo
// SFINAE-compatible wrapper for `std::to_address()`.
//
// The standard does not require `std::to_address()` to be SFINAE-compatible
// when code attempts instantiation with non-pointer-like types, and libstdc++'s
// implementation hard errors. For the sake of templated code that wants simple,
// unified handling, Chromium instead uses this wrapper, which provides that
// guarantee. This allows code to use "`to_address()` would be valid here" as a
// constraint to detect pointer-like types.
namespace partition_alloc::internal::base {
// Note that calling `std::to_address()` with a function pointer renders the
// program ill-formed.
template <typename T>
requires(!std::is_function_v<T>)
constexpr T* to_address(T* p) noexcept {
return p;
}
// These constraints cover the cases where `std::to_address()`'s fancy pointer
// overload is well-specified.
template <typename P>
requires requires(const P& p) { std::pointer_traits<P>::to_address(p); } ||
requires(const P& p) { p.operator->(); }
constexpr auto to_address(const P& p) noexcept {
return std::to_address(p);
}I would have liked to avoid requesting the review of one more reviewer.
Maybe the definition of `to_address` could be part of span.h private functions?
Then you can remove the PRESUBMIT change, the "to_address.h" file.
// We also exclude Android because PartitionAlloc's death test macros are
// disabled there (see partition_alloc_base/test/gtest_util.h).I am not sure to understand. Do we need to even use `PA_USE_DEATH_TESTS`?
I believe this is a legacy from chromium. If the test use the macro from the file you are referencing: partition_alloc/partition_alloc_base/test/gtest_util.h
then the behavior of the macro becomes a no-op under Android.
I was thinking span.h could avoid using it (a straight removal if the #if, but keep the content)
Related details
- Sergio Solano
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
// We also exclude Android because PartitionAlloc's death test macros are
// disabled there (see partition_alloc_base/test/gtest_util.h).I am not sure to understand. Do we need to even use `PA_USE_DEATH_TESTS`?
I believe this is a legacy from chromium. If the test use the macro from the file you are referencing: partition_alloc/partition_alloc_base/test/gtest_util.h
then the behavior of the macro becomes a no-op under Android.I was thinking span.h could avoid using it (a straight removal if the #if, but keep the content)
+1 I don't think we should need to switch this, there should already be some proper combination for the exclusion we want to handle.
I think I missed that this condition was modified.
Related details
- Sergio Solano
Sergio Solano (Gerrit)
Sergio Solano added 8 comments![]( "Open in Gerrit")
Hi guys, here is the new version. It includes a refactored to_address.h file that avoids using `std::to_address` to avoid changes in PRESUBMIT.py. If you agree we could use this version and maybe consider applying the same change in base.
// allocation (it is not the pointer toone-past-the-end).Sergio Solanonit: "to one"
Done
// `array` has three elmenents, so `span` has three elements, soSergio Solanotypo
Done
// `array` has three elmenents, so `span` has three elements, soSergio Solanotypo
Done
// `array` has three elmenents, so `span` has three elements, soSergio Solanotypo
Done
// SFINAE-compatible wrapper for `std::to_address()`.
//
// The standard does not require `std::to_address()` to be SFINAE-compatible
// when code attempts instantiation with non-pointer-like types, and libstdc++'s
// implementation hard errors. For the sake of templated code that wants simple,
// unified handling, Chromium instead uses this wrapper, which provides that
// guarantee. This allows code to use "`to_address()` would be valid here" as a
// constraint to detect pointer-like types.
namespace partition_alloc::internal::base {
// Note that calling `std::to_address()` with a function pointer renders the
// program ill-formed.
template <typename T>
requires(!std::is_function_v<T>)
constexpr T* to_address(T* p) noexcept {
return p;
}
// These constraints cover the cases where `std::to_address()`'s fancy pointer
// overload is well-specified.
template <typename P>
requires requires(const P& p) { std::pointer_traits<P>::to_address(p); } ||
requires(const P& p) { p.operator->(); }
constexpr auto to_address(const P& p) noexcept {
return std::to_address(p);
}I would have liked to avoid requesting the review of one more reviewer.
Maybe the definition of `to_address` could be part of span.h private functions?
Then you can remove the PRESUBMIT change, the "to_address.h" file.
Done, i used the internal namespace instead of a private member function so that both the fixed-extent and dynamic-extent span definitions can use it without duplication.
// SFINAE-compatible wrapper for `std::to_address()`.
//
// The standard does not require `std::to_address()` to be SFINAE-compatible
// when code attempts instantiation with non-pointer-like types, and libstdc++'s
// implementation hard errors. For the sake of templated code that wants simple,
// unified handling, Chromium instead uses this wrapper, which provides that
// guarantee. This allows code to use "`to_address()` would be valid here" as a
// constraint to detect pointer-like types.
namespace partition_alloc::internal::base {
// Note that calling `std::to_address()` with a function pointer renders the
// program ill-formed.
template <typename T>
requires(!std::is_function_v<T>)
constexpr T* to_address(T* p) noexcept {
return p;
}
// These constraints cover the cases where `std::to_address()`'s fancy pointer
// overload is well-specified.
template <typename P>
requires requires(const P& p) { std::pointer_traits<P>::to_address(p); } ||
requires(const P& p) { p.operator->(); }
constexpr auto to_address(const P& p) noexcept {
return std::to_address(p);
}I would have liked to avoid requesting the review of one more reviewer.
Maybe the definition of `to_address` could be part of span.h private functions?
Then you can remove the PRESUBMIT change, the "to_address.h" file.
Done. I've switched the implementation to the recursive version we discussed. This version avoids the literal std::to_address call and the need to modify PRESUBMIT.py.
I've also updated the comments to_address.h to also bypass the automated presubmit check.
// We also exclude Android because PartitionAlloc's death test macros are
// disabled there (see partition_alloc_base/test/gtest_util.h).Stephen NuskoI am not sure to understand. Do we need to even use `PA_USE_DEATH_TESTS`?
I believe this is a legacy from chromium. If the test use the macro from the file you are referencing: partition_alloc/partition_alloc_base/test/gtest_util.h
then the behavior of the macro becomes a no-op under Android.I was thinking span.h could avoid using it (a straight removal if the #if, but keep the content)
+1 I don't think we should need to switch this, there should already be some proper combination for the exclusion we want to handle.
I think I missed that this condition was modified.
Thanks. At some point i ran into Android error and that fixed it, somehow i cannot recreate the errors.
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
2 comments
PA_EXPECT_CHECK_DEATH(debug::Alias(&fixed_span[3u]));
PA_EXPECT_CHECK_DEATH(debug::Alias(&dyn_span[3u]));
PA_EXPECT_CHECK_DEATH(debug::Alias(fixed_span.get_at(3u)));
PA_EXPECT_CHECK_DEATH(debug::Alias(dyn_span.get_at(3u)));If we no longer need to have a macro around it, can you move this back up
So that we have the EXPECT_CHECK_DEATH of the span up with the other accesses?
Done. I've switched the implementation to the recursive version we discussed. This version avoids the literal std::to_address call and the need to modify PRESUBMIT.py.
I've also updated the comments to_address.h to also bypass the automated presubmit check.
This is very weird, couldn't we just send a separate CL just to add to_address and the presubmit and then import span?
But I'm fine with landing this as is and then perhaps as a follow up getting the presubmit exception in a separate review?
Related details
- Arthur Sonzogni
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano added 2 comments![]( "Open in Gerrit")
PA_EXPECT_CHECK_DEATH(debug::Alias(&fixed_span[3u]));
PA_EXPECT_CHECK_DEATH(debug::Alias(&dyn_span[3u]));
PA_EXPECT_CHECK_DEATH(debug::Alias(fixed_span.get_at(3u)));
PA_EXPECT_CHECK_DEATH(debug::Alias(dyn_span.get_at(3u)));If we no longer need to have a macro around it, can you move this back up
So that we have the EXPECT_CHECK_DEATH of the span up with the other accesses?
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+1![]( "Open in Gerrit")
- Sergio Solano
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
🎉 YAY!
Related details
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Making span.h available within partition_alloc.
Added span.h and span_unittests.cc with their dependencies.
Design doc: go/partition-alloc-spanification
BYPASS_RECITATION_REASON=Internal file copy from base to partition_alloc
- M base/allocator/partition_allocator/src/partition_alloc/BUILD.gn
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/compiler_specific.h
- A base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/containers/checked_iterators.h
- A base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/containers/span.h
- A base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/containers/span_forward_internal.h
- A base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/containers/span_unittest.cc
- A base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/types/to_address.h
Code-Review: +1 by Arthur Sonzogni, +1 by Stephen Nusko
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |