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