[PATCH] D35422: Add MemoryMappedSection struct for two-level memory map iteration

[Francis Ricci via Phabricator via llvm-commits]

fjricci added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_procmaps.h:60
+
+#if SANITIZER_MAC
+
----------------
kcc wrote:
> fjricci wrote:
> > kcc wrote:
> > > Folks, please avoid ifdefs like this. 
> > > I'd appreciate if you can refactor the code to get rid of this ifdef (e.g move stuff to a separate file)
> > This file already has quite a few OS-based #ifdefs in it (which is why I didn't think adding another would be problematic). Is it worth trying to refactor in a way that they all go away, or just this one in particular?
> > already has quite a few OS-based #ifdefs
> 
> Not too many in fact. 
> And when there are N bad things in a file it's not always an excuse to add (N+1)-th
> 
> > Is it worth trying to refactor in a way that they all go away
> 
> Yes, please. It's the same amount of work anyway.
> E.g. declare a private object 
>   MemoryMappingLayoutRep *rep
> and implement MemoryMappingLayoutRep in OS-specific files. 
Makes sense. Will work on this. I wonder if we could get things to a point where we started enforcing lint rules against platform-specific ifdefs (unless some are actually unavoidable).


Repository:
  rL LLVM

https://reviews.llvm.org/D35422