Details
- Reviewers
kcc eugenis alekseyshl vitalybuka filcab
Diff Detail
- Repository
- rL LLVM
Event Timeline
I split the changes strictly along subdirectory lines, but some of the stuff here only makes sense in light of the ASan port change.
Please let me know if you'd like me to slice this up into smaller pieces or recombine it and the ASan change differently.
lib/sanitizer_common/sanitizer_common_libcdep.cc | ||
---|---|---|
43 | In Fuchsia the output will never go into a file or somewhere where colors aren't supported? | |
lib/sanitizer_common/sanitizer_file.cc | ||
20 | Usual nit: sanitizer_platform.h -> Test for SANITIZER_FUCHSIA. | |
lib/sanitizer_common/sanitizer_fuchsia.cc | ||
16 | Assuming sanitizer_fuchsia.h has a proper guard, I'm ok with this as is, not too far from including sanitizer_platform.h only. | |
lib/sanitizer_common/sanitizer_fuchsia.h | ||
16 | Avoid parsing useless includes by always including sanitizer_platform.h first and testing for the platform you want. | |
lib/sanitizer_common/sanitizer_platform.h | ||
279 | Doesn't seem Fuchsia related. | |
lib/sanitizer_common/sanitizer_platform_interceptors.h | ||
25 | NOT_WINDOWS doesn't mean UNIX, though. We do have SI_POSIX, which might be close to what you need? | |
26 | This seems highly misleading. | |
33 | Why not #if SI_POSIX? Or maybe add some additional OS there. | |
120 | Where did this go? | |
lib/sanitizer_common/sanitizer_stacktrace_printer.cc | ||
36 | Maybe have a different default? Or are you planning on simply not having this type of functionality (allow users to set their stacktrace format)? | |
131 | Same as above. | |
lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc | ||
17 | Usual comment. Don't parse headers you don't need. |
Looks good in general, but can't comment much on the OS itself :-)
I have a few changes I'd like to see, though.
lib/sanitizer_common/sanitizer_common_libcdep.cc | ||
---|---|---|
43 | Correct. Fuchsia's logging will always go through a presentation filter that can massage the colorization as appropriate for its final output format. | |
lib/sanitizer_common/sanitizer_platform.h | ||
279 | It's related in the sense that it's cleaning up an OS-specific assumption. | |
lib/sanitizer_common/sanitizer_platform_interceptors.h | ||
25 | It looks like SANITIZER_POSIX does in fact so far just mean !SANITIZER_WINDOWS. | |
120 | Oops. Restored. | |
lib/sanitizer_common/sanitizer_stacktrace_printer.cc | ||
36 | We won't have that functionality at all. |
In Fuchsia the output will never go into a file or somewhere where colors aren't supported?