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