mstorsjo wrote: These changes mostly look good to me, but it all feels a bit unclear to me still. So I'd appreciate e.g. one or a few sentences in the commit message on each of these commits, to explain a bit about the why/how and how things work in other preexisting environments. Is this for building LLVM/Clang with GCC, or for building LLVM/Clang with Clang?
E.g. for "Fix Signals compatibility with Cygwin API", what error/warning do we get without this? What system data structure has a different definition on Cygwin compared with other common unixes, that causes this incompatibility? (Also, for that change, maybe it'd be more idiomatic C++ code with `static_cast<const char *>()`.) For "Fix symbol visibility definition" - is this about using the recent mingw-originated feature of setting symbol visibility for excluding symbols from autoexport? I.e. this is for when building LLVM with Clang? The commit message "fix" is quite vague. "Disable shared libs on Cygwin by default" mostly looks reasonable, it's something where it makes sense to do whatever we've set up to do for mingw. But even there it would be nice with one sentence saying what issue you were running into before doing this change. "Remove erroneous define and clean it up" - a mention of what the problem is, maybe some digging of where this code originated, and a mention of why it's not relevant any longer would be good. "Avoid unwanted shared libclang builds" - Also here, mention what happens right now (and under which circumstances). Then secondly, LLVM only allows "squash and merge" merging of PRs - so all these nicely split changes (with individual explanations soon, I'd hope!) would end up all mushed together. So unfortunately, to preserve the info about each of them, they'd need to be filed in separate individual PRs. Luckily most of these changes don't build on top of each other (even if you might not be able to build things properly unless you have all of them applied at once?), so it should be fairly straightforward to file all those PRs in parallel. Most of these should be easy to approve and merge if filed separately with a couple of sentences each explaining the fix. https://github.com/llvm/llvm-project/pull/134494 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
