Skip to content

Conversation

@hughbe
Copy link
Contributor

@hughbe hughbe commented Nov 28, 2016

  • Control path uncovered warning
  • Non accessible method error
  • Can't friend class error
  • Sign conversion warnings
  • Internal stdlib assertitions when compiling swift code

Copy link
Contributor

@rudkx rudkx left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM but @eeckstein should probably take a quick look.

Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation is off here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I also removed the double new line

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 the right way to fix this is just friend SCCVisitor;, not to make the visit method public.

Copy link
Contributor Author

@hughbe hughbe Dec 2, 2016

Choose a reason for hiding this comment

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

I get an error: “class template has already been declared as a non-class template”

I think the fix is to change this to friend class SCCVisitor<IVInfo> which compiles in VS. I'm running a full build now to make sure.

EDIT: friend SCCVisitor also works, so I went for your suggestion instead

Copy link
Contributor

Choose a reason for hiding this comment

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

SetVector has a getArrayRef method you can use instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, I also removed the MSVC comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Please wrap to 80 columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@eeckstein
Copy link
Contributor

LGTM

@jrose-apple
Copy link
Contributor

@swift-ci Please smoke test and merge

@hughbe
Copy link
Contributor Author

hughbe commented Dec 2, 2016

This didn't get merged either although tests passed!

@jrose-apple jrose-apple merged commit 63db004 into swiftlang:master Dec 2, 2016
@hughbe hughbe deleted the siloptimizer-msvc branch December 2, 2016 21:12
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.

4 participants