[PATCH] D35135: Refactor MemoryMappingLayout::Next to use a single struct instead of output parameters. NFC.

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_procmaps.h:35
+struct MemoryMappedSegment {
+ public:
+  MemoryMappedSegment(char *buff = nullptr, uptr size = 0)
----------------
This is struct, everything is public by default.


================
Comment at: lib/sanitizer_common/sanitizer_procmaps.h:39
+  ~MemoryMappedSegment() {}
+
+  uptr start;
----------------
I think it might improve readability a bit if we add IsReadable(), IsWriteable(), IsExecutable() and IsShared() functions here (although latter is not needed at the moment).


================
Comment at: lib/sanitizer_common/sanitizer_procmaps.h:86
+  void GetSegmentAddrRange(MemoryMappedSegment *segment, uptr vmaddr,
+                           uptr vmsize);
   int current_image_;
----------------
Nit: what would you say about moving output parameter to be the last one and renaming function to Set...?

  void SetSegmentAddrRange(uptr vmaddr, uptr vmsize, MemoryMappedSegment *segment);

Also, it's not that big, it is called from one place only, maybe just inline it? We do not have GetSegmentFileName and such after all.


================
Comment at: lib/sanitizer_common/sanitizer_procmaps_common.cc:123
   InternalScopedString module_name(kMaxPathLength);
-  for (uptr i = 0; Next(&cur_beg, &cur_end, &cur_offset, module_name.data(),
-                        module_name.size(), &prot);
-       i++) {
-    const char *cur_name = module_name.data();
+  MemoryMappedSegment segment(module_name.data(), kMaxPathLength);
+  for (uptr i = 0; Next(&segment); i++) {
----------------
kMaxPathLength -> module_name.size()


================
Comment at: lib/sanitizer_common/sanitizer_procmaps_mac.cc:109
+        (current_filetype_ == /*MH_EXECUTE*/ 0x2) ? sc->vmaddr : sc->fileoff;
+    Printf("filename: %p\n", segment->filename);
+    if (segment->filename) {
----------------
Remove it


================
Comment at: lib/sanitizer_common/sanitizer_procmaps_mac.cc:240
+bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
+  Printf("filename after call: %p\n", segment->filename);
   for (; current_image_ >= kDyldImageIdx; current_image_--) {
----------------
Remove it


https://reviews.llvm.org/D35135