[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

Thu Oct 28 08:43:10 PDT 2021

thakis added inline comments.

================
Comment at: clang/include/clang/Basic/Builtins.def:1059
+#undef strcasecmp
+#undef strncasecmp
+
----------------
GMNGeoffrey wrote:
> chandlerc wrote:
> > thakis wrote:
> > > Why do we need this with bazel but not with other windows builds?
> > I don't know how this never was hit by other builders. I don't usually develop on Windows so I have very little experience with different builds there.
> > 
> > My guess is that it is the particular way that Bazel+clang-cl compile on Windows causes sligthtly more to be transitively included with `#include <windows.h>` and that grows the number of functions hit with this. I thought about trying to "fix" that, but the existing `#undef` lines made me think it wouldn't be completely successful.
> > 
> > Are you worried about the change? Looking for a different fix?
> Yeah I want to note that my review is basically only for the Bazel stuff. This looked fine to me based on the existing `undef`s, but mostly trusting Chandler to determine that this is OK and isn't in the territory of the Bazel build requiring unsavory things in the core project.
Worried is a strong word :) It just feels off. The redundant build system config changes shouldn't need changes to LLVM's code itself imho. If there's some difference in build behavior here, it feels like we should fix that on the build config side of things, else we'll have weird one-off fixes like this in other places potentially. I feel we should at least understand what's going on.

(This isn't a terribly strong opinion fwiw.)

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112399/new/

https://reviews.llvm.org/D112399