GMNGeoffrey added inline comments.
================ Comment at: clang/include/clang/Basic/Builtins.def:1059 +#undef strcasecmp +#undef strncasecmp + ---------------- 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. ================ Comment at: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl:33 + for impl_name in [dll_name, dylib_name, so_name]: + cc_binary( + name = impl_name, ---------------- chandlerc wrote: > GMNGeoffrey wrote: > > What about this makes `binary_alias` no longer work? > The `output_group` in the subsequent `filegroup`. It requires the actions in > the sources to have a correctly named `output_group`, which `cc_binary` with > `linkshared` does. > > It's possible that I could just rename things enough by adding multiple > layers of file groups and maybe some added "copy this file" rules... But I'm > worried that wouldn't work well long term. For example, it seems reasonable > for the `.lib` file to *name* the `.dll` file that should be loaded. And so > if we build it with `cc_binary` that has the wrong name, I'm worried things > won't work as expected. I have higher confidence in this arrangement where > the `cc_binary` directly produces the desired name. > > Something like that actually happened when I tried a different way of wiring > this up that still used the `binary_alias` on Linux -- it ended up actually > loading `liblibclang_impl.so` and not `libclang.so`. That was a different > setup, so maaaybe I wouldn't hit that exact situation, but its the kind of > thing that inclined me towards this model. That's sufficiently convincing. Hopefully some layer will figure out these are all the same and not build the same thing 3 times 🤞 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112399/new/ https://reviews.llvm.org/D112399 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
