[blink] Switch wtf/allocator/* to "blink" namespace [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 5 comments![]( "Open in Gerrit")
@tk...@chromium.org ended up with 66 files, but no more "using"
should we continue?
namespace WTF {
using blink::NotNullTag;
namespace internal {
using blink::internal::__thisIsHereToForceASemicolonAfterThisMacro;
} // namespace internalHelmut JanuschkaCan we avoid to add these `using`? Do we have to update too many files if we drop them?
done, 15 files.
namespace WTF {
using blink::PartitionAllocator;
} // namespace WTFHelmut JanuschkaCan we avoid to add this `using`? Do we have to update too many files if we drop it?
11 files
namespace WTF {
using blink::Partitions;
} // namespace WTFHelmut JanuschkaCan we avoid to add this `using`? Do we have to update too many files if we drop it?
done, alot files
using blink::PartitionAllocator;Helmut JanuschkaCan we avoid to add this `using`? Do we have to update too many files if we drop it?
Done
Related details
- Kent Tamura
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@tk...@chromium.org ended up with 66 files, but no more "using"
should we continue?
ahh wanted to keep this open
Related details
- Kent Tamura
Code-Coverage
Code-Review
No-Unresolved-Comments
Kent Tamura (Gerrit)
Kent Tamura added 3 comments![]( "Open in Gerrit")
Helmut Januschka@tk...@chromium.org ended up with 66 files, but no more "using"
should we continue?
ahh wanted to keep this open
IMO, a CL touching 100+ files should be split.
This CL touches less-than-100 files, however I recommend to split this to one for `allocator.h`, one for `partition_allocator.h`, and one for `partitions.h`.
https://issues.chromium.org/issues/422768753#comment1
> 1. Choose a **single** header in platform/wtf/
void operator()(uint8_t* buffer) { blink::Partitions::BufferFree(buffer); }We should remove `blink::` prefix because this code is in the "blink" namespace.
Please re-check all the files in the CL for unnecessary `blink::` prefixes.
using base::NotNullTag;This CL **removes** `WTF::NotNullTag` instead of moving it to `blink` namespace.
It's a reasonable change, but not in the scope of crbug.com/422768753, so if you'd like to remove `WTF::NotNullTag`, it should be done in a separated CL.
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
Helmut Januschka@tk...@chromium.org ended up with 66 files, but no more "using"
should we continue?
Kent Tamuraahh wanted to keep this open
IMO, a CL touching 100+ files should be split.
This CL touches less-than-100 files, however I recommend to split this to one for `allocator.h`, one for `partition_allocator.h`, and one for `partitions.h`.https://issues.chromium.org/issues/422768753#comment1
> 1. Choose a **single** header in platform/wtf/
I'm having a hard time splitting this up (especially the "single header per CL" requirement).
The NotNullTag was straightforward: http://crrev.com/c/6632732, but I'm struggling with the others.
When trying to isolate them, I always end up in a "this-depends-on-this" nightmare.
Could we tackle headers on a per-component basis instead? Like handling /wtf/allocator/* (and everything that throws errors) as one unit? Or do you have any suggestions for how to handle this without adding tons of "using" statements in the split CLs?
void operator()(uint8_t* buffer) { blink::Partitions::BufferFree(buffer); }We should remove `blink::` prefix because this code is in the "blink" namespace.
Please re-check all the files in the CL for unnecessary `blink::` prefixes.
Done
using base::NotNullTag;This CL **removes** `WTF::NotNullTag` instead of moving it to `blink` namespace.
It's a reasonable change, but not in the scope of crbug.com/422768753, so if you'd like to remove `WTF::NotNullTag`, it should be done in a separated CL.
- Kent Tamura
Kent Tamura (Gerrit)
Kent Tamura added 1 comment![]( "Open in Gerrit")
Helmut Januschka@tk...@chromium.org ended up with 66 files, but no more "using"
should we continue?
Kent Tamuraahh wanted to keep this open
Helmut JanuschkaIMO, a CL touching 100+ files should be split.
This CL touches less-than-100 files, however I recommend to split this to one for `allocator.h`, one for `partition_allocator.h`, and one for `partitions.h`.https://issues.chromium.org/issues/422768753#comment1
> 1. Choose a **single** header in platform/wtf/
I'm having a hard time splitting this up (especially the "single header per CL" requirement).
The NotNullTag was straightforward: http://crrev.com/c/6632732, but I'm struggling with the others.
When trying to isolate them, I always end up in a "this-depends-on-this" nightmare.Could we tackle headers on a per-component basis instead? Like handling /wtf/allocator/* (and everything that throws errors) as one unit? Or do you have any suggestions for how to handle this without adding tons of "using" statements in the split CLs?
Would you try:
1. Make a CL to move wtf/allocator/allocator.h to blink namespace, ask for review, and merge
2. Make a CL to move wtf/allocator/partition_allocator.h to blink namespace, ask for review, and merge
3. Make a CL to move wtf/allocator/partitions.h to blink namespace, ask for review, and merge
If it's difficult to follow these steps, it's ok to make a single CL for these three headers.
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |