Skip to content

Fixed implicit closure in sorting comparator#827

Closed
stelleg wants to merge 1 commit intoflexflow:repo-refactorfrom
stelleg:repo-refactor
Closed

Fixed implicit closure in sorting comparator#827
stelleg wants to merge 1 commit intoflexflow:repo-refactorfrom
stelleg:repo-refactor

Conversation

@stelleg
Copy link
Contributor

@stelleg stelleg commented Jul 3, 2023

Description of changes:
Existing code requires implicit closure over f, which appears to be invalid C++ and fails with g++.

Related Issues:

Linked Issues:

  • Issue #

Issues closed by this PR:

  • Closes #

Before merging:

  • Did you update the flexflow-third-party repo, if modifying any of the Cmake files, the build configs, or the submodules?

@lockshaw
Copy link
Collaborator

lockshaw commented Jul 6, 2023

So this was the fix I had--it seems cleaner, so I'd probably lean toward this unless you have an objection:

 template <typename T, typename F>
 void inplace_sorted_by(std::vector<T> &v, F const &f) {
-  struct {
-    bool operator()(T const &lhs, T const &rhs) { return f(lhs, rhs); }
-  } custom_comparator;
+  auto custom_comparator = [&](T const &lhs, T const &rhs) -> bool {
+    return f(lhs, rhs);
+  };
   
   std::sort(v.begin(), v.end(), custom_comparator);

It's not much code though, so I don't have particularly strong opinions.

@stelleg
Copy link
Contributor Author

stelleg commented Jul 6, 2023

Yep, I prefer yours as well.

@lockshaw
Copy link
Collaborator

lockshaw commented Jul 6, 2023

Great, can you update this PR to have that change and then I'll approve?

Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

see above

@lockshaw
Copy link
Collaborator

Nevermind, fixed in #850

@lockshaw lockshaw closed this Jul 10, 2023
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