Port swift/basic to Windows using MSVC by hughbe · Pull Request #5949 · swiftlang/swift

hughbe · 2016-11-28T17:09:47Z

The UUID stuff is the heart of this PR, and relies on linking rpcrt4.lib, found in #5904. We can merge these PRs in any order, however.
The only limitation is that we can't generate a UUID based on the current time, but this isn't terrible.

compnerd · 2016-11-28T17:14:56Z

What if you are using msvcprt with clang-cl?

Right, that was my concern. Would || (defined(_MSC_VER) && !defined(__clang__) work? I dunno how to detect clang-cl!

Maybe its a version thing - we should check _MSC_VER >= X?

The value for _MSC_VER will be the same across clang and clang-cl. What exactly are you trying to protect against?

Well if we don't have this check, I get a fatal error saying "not implemented" at compile time. We need this, but need to figure out how to detect msvcprt

Okay - I've addressed this by changing it to:

#if _LIBCPP_VERSION || (defined(_MSC_VER) && !defined(__clang__))

This works, since this works __has_feature(is_trivially_copyable) with clang-cl

compnerd · 2016-11-28T17:15:16Z

Why the additional _WIN32 check?

no idea, I'll remove when we get a better idea about the other stuff

compnerd · 2016-11-28T17:17:15Z

Hmm, I wonder if there is a better way to handle this. Something like using a __pragma(warning(supress:????)))). In that case, we could have a SWIFT_USED macro.

This is the only place __attribute((used)) is used in the entire library I think, so I'm not sure its worth the effort!

What does LLVM_ATTRIBUTE_USED do?

compnerd · 2016-11-28T17:17:51Z

Might be nice to #define WIN32_LEAN_AND_MEAN here.

compnerd · 2016-11-28T17:18:45Z

Why the whitespace changes?

cleanliness and consistency

The old way is meant to show #if nesting.

right - will revert.

jckarter · 2016-11-28T18:26:58Z

I believe that the only place we use UUIDs in the compiler is to print "opened archetypes" in SIL in a way that lets them be reliably parsed back as unique types. We could probably use a better uniquing scheme in SIL printing now that we have better tracking of opened type uses in SIL, and kill the UUID dependency.

hughbe · 2016-11-28T22:00:23Z

Interesting @jckarter... I've opened SR-3285 to track this. I've filed it as a starter bug. I'm hideously busy atm due so I hope you don't mind that I'm not addressing that in this PR?

jckarter · 2016-11-28T22:06:59Z

@hughbe Sure, no problem.

compnerd · 2016-11-29T01:57:28Z

@jckarter what exactly did you have in mind for replacing the naming scheme with? It sounds simple enough that I might be able to take that on.

jrose-apple · 2016-11-29T17:19:02Z

Nitpick: comments should end with periods. (Although I don't really think you need this one; the alternate formulation isn't so bad that anyone would be tempted to change it.)

Removed the comment as suggested.

jrose-apple · 2016-11-29T17:19:42Z

No need for the comment here.

Removed, thanks

jrose-apple · 2016-11-29T17:20:29Z

memcpy already takes a void *, so you don't need this.

I'm not sure I can entirely remove this. We use this conversion union in methods such as UUIDCompare and UUIDToString that require a UUID passed. I have removed all the conversions from memcpy though as you suggested.

Either ::UUID is a POD type, in which case you can just memcpy into one directly, or it's not, in which case the union isn't safe either.

I had no idea you can just memcpy like that - you learn something new everyday:

This works, and the file builds, so we're good:

#include <rpc.h> int main() { ::UUID uuid1; unsigned char data[16] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16 }; memcpy(&uuid1, data, 16); // uuid1 = {04030201-0605-0807-090A-0B0C0D0E0F10} return 0; }

jrose-apple · 2016-11-29T17:22:03Z

Hm. This isn't 100% correct, then, because these files won't exist when the source root isn't a Git or SVN repo. Maybe we should just have CMake generate empty files in those cases?

Strange - I see lines in the build log on Windows saying "Generating LLVMRevision.inc" etc. etc. for each file. Seems like CMake already does this and has already generated these files by the time Version.cpp is built.

It only does that if you're in a checkout of a Git or SVN repo.

Ah I understand now. I've modified lib/Basic/CMakeLists.txt file to generate an empty file if these files are not found. I've made it a seperate commit because its not necessarily Windows/MSVC specific.

jrose-apple · 2016-11-30T17:29:51Z

These shouldn't be necessary now, since we decided to allow unknown pragmas.

(If we did have them, we should check for Clang instead of for !MSVC.)

I removed the ifs

jrose-apple · 2016-11-30T17:32:09Z

Either ::UUID is a POD type, in which case you can just memcpy into one directly, or it's not, in which case the union isn't safe either.

jrose-apple · 2016-12-02T17:09:55Z

Stray endif() here?

jrose-apple · 2016-12-02T17:16:08Z

@swift-ci Please smoke test and merge

hughbe · 2016-12-02T21:13:52Z

Also this one didn't merge although tests passed

jrose-apple · 2016-12-02T21:22:02Z

sigh

hughbe · 2016-12-15T16:12:36Z

  }

-  using iterator = typename decltype(Data)::iterator;
+  using iterator = typename ArrayRef<PtrTy>::iterator;


https://connect.microsoft.com/VisualStudio/feedback/details/3116518

hughbe · 2016-12-15T16:13:18Z

+    static const size_t Size = (sizeof(void*) - 1) / sizeof(Chunk);
+    static const size_t ActualSize = max<size_t>(Size, 1);
+
+    using MapBase = PrefixMap<Chunk, ValueType, ActualSize>;


https://connect.microsoft.com/VisualStudio/feedback/details/3116197/

hughbe · 2016-12-15T16:14:48Z


  public:
-    using SequenceIterator = EncodedSequence::iterator;
+    using SequenceIterator = typename EncodedSequence::iterator;


This seems to be a Clang deficiency rather than an MSVC deficiency

Agreed, typename ought to be required by the standard. (Assuming EncodedSequence is dependent, of course.)

compnerd reviewed Nov 28, 2016

View reviewed changes

jrose-apple reviewed Nov 29, 2016

View reviewed changes

jrose-apple reviewed Nov 30, 2016

View reviewed changes

Port swift/basic to Windows

56dfb08

jrose-apple reviewed Dec 2, 2016

View reviewed changes

Generate empty *Revision.inc files during the build process

50e94af

jrose-apple approved these changes Dec 2, 2016

View reviewed changes

jrose-apple merged commit a184057 into swiftlang:master Dec 2, 2016

hughbe deleted the basic-win-msvc branch December 9, 2016 14:11

hughbe commented Dec 15, 2016

View reviewed changes

hughbe mentioned this pull request Dec 22, 2016

Fix linking libuuid on Windows #6460

Merged

Conversation

hughbe commented Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hughbe Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Nov 28, 2016

Uh oh!

hughbe commented Nov 28, 2016

Uh oh!

jckarter commented Nov 28, 2016

Uh oh!

compnerd commented Nov 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hughbe Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hughbe commented Nov 28, 2016 •

edited

Loading

hughbe Nov 30, 2016 •

edited

Loading

hughbe Nov 30, 2016 •

edited

Loading

hughbe Dec 2, 2016 •

edited

Loading

hughbe commented Dec 2, 2016 •

edited

Loading

jckarter Dec 15, 2016 •

edited

Loading