[PATCH] D35866: [sanitizer_common] Fuchsia-specific implementation of SanitizerCoverage

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_coverage_fuchsia.cc:32
+#include "sanitizer_internal_defs.h"
+#if SANITIZER_FUCHSIA
+
----------------
filcab wrote:
> Include `sanitizer_platform.h` then immediately test for `SANITIZER_FUCHSIA`. That way you don't re-parse as many files.
Other files are inconsistent in whether they do this, so I took the one that's shorter.
But I'm happy to do this change.


================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc:17
 #include "sanitizer_symbolizer.h"
+#if !SANITIZER_FUCHSIA
 
----------------
filcab wrote:
> filcab wrote:
> > vitalybuka wrote:
> > > To other reviewers:
> > > Sanitizers are inconsistent here. Some files are disabled in cmake other files with such #ifs. Why is it so?
> > Same as above.
> Indeed. I think we should use CMake whenever the test is easy (host OS, target os, etc). But some tests are more involved and are better off being done after all the includes, I guess.
>From what I've seen, the vast majority of cases are unconditional in cmake and use #if around the whole file, so that's the model I followed.



Repository:
  rL LLVM

https://reviews.llvm.org/D35866