Page MenuHomePhabricator

compiler-rt/lib/builtins: s/#include <windows.h>/#include <Windows.h>/g to match proper case.
AbandonedPublic

Authored by plotfi on May 16 2019, 2:37 PM.

Details

Summary

The proper case of Windows.h is with a capital W. On case sensitive filesystems "#include <windows.h" breaks.

Diff Detail

Event Timeline

plotfi created this revision.May 16 2019, 2:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 16 2019, 2:37 PM
Herald added subscribers: Restricted Project, dberris. · View Herald Transcript

This unfortunately differs from llvm and clang :-(. I'd love to see this be corrected though

smeenai added a subscriber: smeenai.

I believe MinGW lowercases all their headers, so that's the convention LLVM has been following, since cross-compilation using the Windows SDK generally involves some sort of case correction (through VFS overlays or CIOPFS or similar) anyway.

Yes, MinGW headers are all lowercase.

And the Windows SDK, where the file is spelled with a capital W, isn't self-consistent (some headers include others with a different capitalization than the files actually are stored with), so to use the official Windows SDK on a case sensitive file system, you need some sort of case fixup anyway. (Or did that change very recently?) And in that case, making it all lowercase is the simplest, which also happens to match the MinGW headers.

plotfi updated this revision to Diff 200067.May 17 2019, 11:11 AM

adding all windows.h's in all of monorepo

Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript

Yes, MinGW headers are all lowercase.

And the Windows SDK, where the file is spelled with a capital W, isn't self-consistent (some headers include others with a different capitalization than the files actually are stored with), so to use the official Windows SDK on a case sensitive file system, you need some sort of case fixup anyway. (Or did that change very recently?) And in that case, making it all lowercase is the simplest, which also happens to match the MinGW headers.

We can't control the case mismatches in the Windows SDK but we can control them in LLVM. Why should we consistently use the wrong case? The difference between LLVM and the Windows SDK using the wrong case is, that we can maintain a vfs overlay file for the Windows SDK only and be fairly certain that it wont change that frequently.

I have no objection to this; but I'm not really confident that this won't break more than it solves.

I have no objection to this; but I'm not really confident that this won't break more than it solves.

Actually, I think the existing VFS overlay would fix this without any changes. So I will likely pull this patch back.

mstorsjo requested changes to this revision.EditedMay 17 2019, 11:41 AM

No, I don't think this is a good idea.

Like I said, the Windows SDK is not self-consistent, so you cannot use it on a case sensitive file system without other case insensitivity fixes anyway. Thus, this change breaks things for any existing user with mingw headers (which works fine on case sensitive file systems without any fixups), and doesn't make the Windows SDK work on a case sensitive filesystem anyway. Once the Windows SDK actually is self-consistent so that it works on a case sensitive filesystem, I'm sure it's possible to add symlinks to mingw to allow the casing that has been decided upon by the Windows SDK by then. But until then, please don't.

As an example, the Windows SDK itself (version 10.0.17763.0) contains 839 occurrances of #include <windows.h> or #include "windows.h" with all lower case, while it contains 1 (one) occurrance of #include <Windows.h>. Now, is it easier to change all those 839 occurrances of windows.h to Windows.h in the SDK itself, or conclude that #include <windows.h> is the canonical spelling?

The SDK contains 455 headers in total where the casing between file name and name used in include directives differs.

In addition, when the linker parses embedded /defaultlib: attributes, those don't match the actual files either. msvcrt.lib contains /defaultlib:kernel32.lib, while the actual file on disk is named kernel32.Lib.

This revision now requires changes to proceed.May 17 2019, 11:41 AM
plotfi abandoned this revision.Wed, May 29, 5:02 PM