Skip to content

Conversation

@hughbe
Copy link
Contributor

@hughbe hughbe commented Nov 28, 2016

  • Uncovered control path errors
  • 32bit/64 bit conversion warnings
  • Unused result warnings
  • Syntax error "template" errors - let me know if I should remove the code that runs for clang currently - it seems like a misplaced template, but I'm no expert

And, don't worry. We're not quite halfway through my PR attack, but for now I'm gonna stop and ease your workload.

@hughbe
Copy link
Contributor Author

hughbe commented Nov 29, 2016

Thanks @compnerd

  • I pushed the #pragma clang diagnostic ignored "-Winconsistent-missing-override" to CMake
  • I removed the "Workaround MSVC warning comments" and updated the content of the llvm_unreachables
  • I sinked the cast and renamed the variable

The variable is used so why is the cast to void needed?

MSVC doesn't think so - I think its because the first check if SWIFT_RT_USE_RegisterPreservingCC which it identifies as a constant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we actually just want to fix these in most cases. Those particular files really are different from the others.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…and yes, that does deserve a comment in those files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted this commit, removed the MSVC checks (as clang-cl needs them but defined MSC_VER). We also now ignore unknown pragmas so this is pointless.
I also added a TODO to the pragmas that I shall try to address once I'm not so busy. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template shouldn't be required by Clang, either, since no explicit template arguments are being given. We should be able to unconditionally remove it from both of these places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I tried once. It really doesn't make sense with the way these files are using overrides—they're coming from templates. (Or macros, I can't remember.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always a reason. I reverted the TODOs. There are no changes to the pragmas in this PR

@jrose-apple
Copy link
Contributor

@jckarter, does this seem all right to you now?

@swift-ci Please smoke test

@jckarter
Copy link
Contributor

LGTM.

@jrose-apple jrose-apple merged commit 1aa951d into swiftlang:master Nov 30, 2016
@hughbe hughbe deleted the irgen-msvc branch December 16, 2016 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants