[PATCH] D54904: Introduce `AddressSpaceView` template parameter to `SizeClassAllocator32`, `FlatByteMap`, and `TwoLevelByteMap`.

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

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