Fix errors and warnings build swift/clangImporter using MSVC on Windows #5950
Conversation
| #pragma mark Internal data structures | ||
| #endif |
There was a problem hiding this comment.
This is pretty awful. Does MSVC really not ignore pragmas it doesn't understand? If so, maybe we should just use comments instead. @DougGregor, @milseman, any opinions?
There was a problem hiding this comment.
It reports warnings. The reason why I didn't do if defined(__clang__) is because MSVC is the only guy out of the lot not to simply ignore
There was a problem hiding this comment.
Can you disable the unknown pragma warning when building with MSVC?
There was a problem hiding this comment.
I can... I'm personally not a fan of disabling warnings, they exist for a reason. That said, considering MSVC is "pretty awful", it could make sense
There was a problem hiding this comment.
Not every warning exists for a practical reason for all projects. #pragma mark is a pretty common de-facto standard for IDE navigation, and it would be impractical to guard every use of it.
There was a problem hiding this comment.
You're right. I've updated #5904 to silence the warning, and updated this PR to remove all of these ifdefs
| @@ -618,8 +621,12 @@ static bool isAccessibilityConformingContext(const clang::DeclContext *ctx) { | |||
|
|
|||
| bool | |||
| importer::shouldImportPropertyAsAccessors(const clang::ObjCPropertyDecl *prop) { | |||
| // TODO (hughbe/MSVC): fix an error using SwiftImportPropertyAsAccessorsAttr | |||
| // Fails with: "SwiftImportPropertyAsAccessorsAttr" is not a member of 'clang' | |||
There was a problem hiding this comment.
Are you sure you're using swift-clang's 'stable' branch and not upstream Clang?
There was a problem hiding this comment.
Ah yes, this would be it. I'll remove this workaround and make sure I'm not breaking my build
| @@ -1094,7 +1094,7 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D, | |||
| // Objective-C categories and extensions don't have names, despite | |||
| // being "named" declarations. | |||
| if (isa<clang::ObjCCategoryDecl>(D)) | |||
| return {}; | |||
| return ImportedName(); | |||
There was a problem hiding this comment.
I'm okay with this change, but is it really necessary? This seems like a pretty common C++11 feature that will keep slipping in.
There was a problem hiding this comment.
Yes, I'm sorry: MSVC reports errors for each construct, no idea why!
| @@ -563,6 +563,9 @@ OptionalTypeKind importer::translateNullability(clang::NullabilityKind kind) { | |||
| case clang::NullabilityKind::Unspecified: | |||
| return OptionalTypeKind::OTK_ImplicitlyUnwrappedOptional; | |||
| } | |||
|
|
|||
| // Work around MSVC warning: not all control paths return a value | |||
| llvm_unreachable("All switch cases are covered"); | |||
There was a problem hiding this comment.
All of these make me think that we really need an LLVM_MSVC_UNREACHABLE macro that expands to llvm_unreachable("silence MSVC warnings") on MSVC and expands to nothing elsewhere.
There was a problem hiding this comment.
You could argue this is good practice even outside of MSVC, since switch exhaustiveness isn't necessarily guaranteed covering all of an enum's constants (unless it happens to have exactly a power of two contiguously-valued elements).
There was a problem hiding this comment.
Right - what I've done is removed the // Work around MSVC warning comments as well, and made the messages more descriptive
There was a problem hiding this comment.
I've also removed the pragma mark ifdefs
There was a problem hiding this comment.
We did this in Clang for a long time too to handle older GCCs that didn't have -Wswitch-enum. It'll be hard to get back in the habit but it doesn't seem like it needs to be MSVC-specific.
There was a problem hiding this comment.
Well, by the end of my flurry of PRs, there shouldn't be any left :) The habit thing still remains
|
@swift-ci Please smoke test |
No description provided.