chandlerc requested review of this revision. chandlerc added a comment. PTAL, and thanks for feedback so far!
================ Comment at: clang/include/clang/Basic/Builtins.def:1059 +#undef strcasecmp +#undef strncasecmp + ---------------- rnk wrote: > thakis wrote: > > 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.) > Does Bazel build clang with modules? That could lead to windows.h > proliferation. > > If you are motivated to investigate, you can re-run the failing compile > command with /showIncludes to work out where the windows.h include comes in. > Take the corresponding object file, build it with cmake ninja, extract the > compile command there, run it with showincludes, and compare the output. I figured this all out. It was right in front of me. New patch fixes. ================ Comment at: utils/bazel/.bazelrc:113-116 # Use Clang's internal warning flags instead of the ones that sometimes map # through to MSVC's flags. build:clang-cl --copt=/clang:-Wall --host_copt=/clang:-Wall build:clang-cl --copt=/clang:-Werror --host_copt=/clang:-Werror ---------------- rnk wrote: > It is true that MSVC's -Wall is Clang's -Weverything, so this needs a prefix. > -Werror could be -WX, but it's just cosmetic. Yeah, I just figured it was more readable in this section to consistently use `/clang:-W`. ================ Comment at: utils/bazel/.bazelrc:125 +# Disable some warnings hit even with `clang-cl` in Clang's own code. +build:clang-cl --copt=/clang:-Wno-inconsistent-dllimport --host_copt=/clang:-Wno-inconsistent-dllimport +build:clang-cl --cxxopt=/clang:-Wno-c++11-narrowing --host_cxxopt=/clang:-Wno-c++11-narrowing ---------------- rnk wrote: > You shouldn't need to prefix warning flags with `/clang:`, those are > supported out of the box. Again, this just seemed more consistent. I can change if you think the other is better though. ================ Comment at: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h:81 /* The LLVM product name and version */ #define BACKEND_PACKAGE_STRING "LLVM 12.0.0git" ---------------- GMNGeoffrey wrote: > rnk wrote: > > Unrelated to your change, but is this stale? > Yes and we don't currently have a good way to keep it up to date. The overall > problem of how to make sure we keep up to date with CMake configure knobs is > unsolved. I wonder if we could run CMake in some limited capacity, but that's > a whole can of worms... I'll just commit an update to this separately. I assume that doesn't need separate review? ;] 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
