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

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_coverage_fuchsia.cc:32
+#include "sanitizer_internal_defs.h"
+#if SANITIZER_FUCHSIA
+
----------------
Include `sanitizer_platform.h` then immediately test for `SANITIZER_FUCHSIA`. That way you don't re-parse as many files.


================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc:17
 #include "sanitizer_symbolizer.h"
+#if !SANITIZER_FUCHSIA
 
----------------
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.


================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc:17
 #include "sanitizer_symbolizer.h"
+#if !SANITIZER_FUCHSIA
 
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D35866