C++/WinRT restore SDK support#122
Conversation
kennykerr
requested review from
BenJKuhn,
DefaultRyan,
Scottj1s and
devhawk
as code owners
|
And no, I have no idea why comments from a previous PR are showing up here. 🙄 |
|
@kennykerr can you delete the comments that are not relevant to this PR? |
|
|
||
| private: | ||
|
|
||
| #if XLANG_PLATFORM_WINDOWS |
There was a problem hiding this comment.
wondering if we should factor out windows support into a separate header...
There was a problem hiding this comment.
Yep, I've been thinking about that. I'll see if I can come up with a reasonable way to separate this out.
| static void add_files_from_xml( | ||
| std::set<std::string>& files, | ||
| std::string const& sdk_version, | ||
| std::experimental::filesystem::path const& xml_path, |
There was a problem hiding this comment.
should we be using std::filesystem::path now?
There was a problem hiding this comment.
Yes, I'd love to. I thought Harry mentioned some reason why that didn't work on the Ubuntu build at present. I can check whether this has been fixed.
There was a problem hiding this comment.
Clang 6.0's libc++ doesn't include filesystem in std namespace. https://github.com/llvm-mirror/libcxx/tree/release_60/include
There was a problem hiding this comment.
Clang 7 has been out for almost 6 months. 😉
| KEY_READ, | ||
| &key)) | ||
| { | ||
| throw_invalid("Could not find the Windows SDK"); |
There was a problem hiding this comment.
I'd prefer distinct error strings here and following, to help diagnosis
| database(database&&) = delete; | ||
| database& operator=(database&&) = delete; | ||
|
|
||
| static bool is_database(std::string_view const& path) |
There was a problem hiding this comment.
consider factoring this and the duplicate ctor logic into a helper
| settings.input = args.files("input"); | ||
| settings.reference = args.files("reference"); | ||
|
|
||
| settings.input = args.files("input", database::is_database); |
There was a problem hiding this comment.
curious why a deep inspection of the PE header is necessary, versus just an extension check?
There was a problem hiding this comment.
Previously, we just checked the extension but I was concerned about this being used with xlang (non-WinRT) projects where there has been talk of using a different file extension.
4 participants
This update restores support for referring a specific SDK rather than just a set of winmd files. All of the following now works as before:
-in sdk-in sdk+-in 10.0.18314.0-in 10.0.18314.0+This update also includes a few bug fixes and tweaks to bring the xlang version on par with the production version of the cppwinrt compiler. I've also ported a few more unit tests.