[PATCH] D28596: [compiler-rt] General definition for weak functions.

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:17
 #include "sanitizer_platform.h"
+#include "sanitizer_win_defs.h"
 
----------------
mpividori wrote:
> alekseyshl wrote:
> > It does not feel right, unconditionally including win specific header in the generic module. I realize that within sanitizer_win_defs.h there is a #if SANITIZER_WINDOWS guard, but still. Can you move it into #if SANITIZER_WINDOWS block, where it is actually needed?
> @alekseyshl  Ok I can change that, but in https://reviews.llvm.org/D28525  they suggested that I simply include the header and only use `#ifdef SANITIZER_WINDOWS` inside the header, to simplify the code. Whatever you prefer is ok for me.
To me, D28525 can be justified by the fact that it's a .cc file containing both win and non-win code, hence both types of headers are there (although I'd rather split it into platform specific files and have a check inside sanitizer_win_defs.h that SANITIZER_WINDOWS is defined, but anyway).

Here you're defining things, you already have those #if blocks, move it into the first one requiring definitions from this file.

Just and idea, you don't have to do anything about it: if we take a step back, whatever definitions use from sanitizer_win_defs.h, are they used anywhere else? My (limited) understandin so far, now the rest of the code is going to use WEAK_DEF macro, everything else is its implementation detail, right? If so, why not move all those definitions to sanitizer_internal_defs.h under #if SANITIZER_WINDOWS? Wouldn't it express better what we're trying to achieve?


https://reviews.llvm.org/D28596