[PATCH] D54904: Introduce `AddressSpaceView` template parameter to `SizeClassAllocator32`, `FlatByteMap`, and `TwoLevelByteMap`.
Kuba (Brecka) Mracek via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 26 10:06:26 PST 2018
kubamracek added a comment.
The patch is quite big. I've added a couple suggestions how to reduce it to make it easier to review.
================
Comment at: lib/sanitizer_common/sanitizer_allocator_bytemap.h:18
// Maps integers in rage [0, kSize) to u8 values.
-template<u64 kSize>
+template <u64 kSize, typename AddressSpaceViewTy = LocalAddressSpaceView>
class FlatByteMap {
----------------
Is this necessary? FlatByteMap isn't doing any dereferences besides reading it's own fields.
================
Comment at: lib/sanitizer_common/sanitizer_allocator_bytemap.h:80
+ if (!map2)
+ return 0;
+ auto value_ptr = AddressSpaceView::Load(&map2[idx % kSize2]);
----------------
Avoid reformatting code we don't need to change.
================
Comment at: lib/sanitizer_common/sanitizer_allocator_internal.h:50
+template <typename AddressSpaceView>
+using PrimaryInternalAllocatorT = SizeClassAllocator32<AP32<AddressSpaceView>>;
+using PrimaryInternalAllocator =
----------------
Can we unify the naming convention of adding "T" and "Ty" suffix to types? Do we need both?
================
Comment at: lib/sanitizer_common/sanitizer_allocator_internal.h:63
+ SecondaryInternalAllocator>
+ InternalAllocator;
----------------
Avoid reformatting code we don't need to change.
================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:243-246
+ for (uptr region = 0; region < kNumPossibleRegions; region++) {
+ uptr class_id = possible_regions[region];
+ if (class_id) {
+ uptr chunk_size = ClassIdToSize(class_id);
----------------
This looks like an optimization. Can we remove it from the patch?
================
Comment at: lib/sanitizer_common/sanitizer_type_traits.h:36-40
+template <typename T, typename U>
+struct is_same : public false_type {};
+
+template <typename T>
+struct is_same<T, T> : public true_type {};
----------------
This (sanitizer_type_traits.h, the test file and the static_assert) can be moved to a separate patch.
Repository:
rCRT Compiler Runtime
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54904/new/
https://reviews.llvm.org/D54904
More information about the llvm-commits
mailing list