Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 142 #173

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Fix issue 142 #173

merged 1 commit into from
Jan 11, 2022

Conversation

fteicht
Copy link
Collaborator

@fteicht fteicht commented Jan 11, 2022

Fix issue #142

@fteicht fteicht linked an issue Jan 11, 2022 that may be closed by this pull request
@fteicht fteicht requested a review from galleon January 11, 2022 10:08
cpp/CMakeLists.txt Show resolved Hide resolved
cpp/src/hub/solver/mcts/mcts.hh Outdated Show resolved Hide resolved
@dbarbier
Copy link
Contributor

@fteicht Your changes break windows-2016 builds; any chance to have a version which works with any windows-* version?

@fteicht
Copy link
Collaborator Author

fteicht commented Jan 11, 2022

Yes because I've just figured out that old versions of MSVC (e.g. 2017 and before) cannot correctly parse typedef members inside lambda functions. It was corrected by the introduction of the Zc:lambda compilation flag but it only works with newest versions of MSVC. Thus I've reverted the commit that simplifies MCTS' FullExpand's constructor implementation.

typedef typename ExecutionPolicy::template atomic<std::size_t>
atomic_size_t;
typedef typename ExecutionPolicy::template atomic<double> atomic_double;
typedef typename ExecutionPolicy::template atomic<bool> atomic_bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

These typedefs appear in public methods, why are they private?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I've just committed the change. Thanks!

Copy link
Contributor

@dbarbier dbarbier left a comment

Choose a reason for hiding this comment

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

Well done, looks good to me.

Since windows-2016 Github runners are going to be removed on March 2022, you should replace windows-2016 by windows-latest on l.40 of build.yml
build = [ "macos-10.15", "ubuntu-latest", "windows-2016" ]
and l. 29 of release.yml
build_dict = { "macos":["macos-10.15"], "ubuntu":["ubuntu-latest"], "windows":["windows-2016"] }
On Windows backward compatibility is ensured, so we can build on -latest.

And bonus points if you squash your commits ;-)

@dbarbier
Copy link
Contributor

@fteicht Please remove the merge commit, it does not provide any value:

$ git diff 45a5132 7bf5f70
$

You can run

 git reset --hard 45a5132
 git push --force-with-lease origin fix-issue-142

@fteicht fteicht removed the request for review from galleon January 11, 2022 14:48
@fteicht fteicht merged commit 93f0283 into airbus:master Jan 11, 2022
@fteicht fteicht deleted the fix-issue-142 branch January 11, 2022 16:59
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.

scikit does not compile with Visual Studio 16 2019
2 participants