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

ordered_set: operator<: Fix for a case of strict inclusion. #3431

Conversation

DoctorNoobingstoneIPresume
Copy link
Contributor

When all elements compared so far in sets x and y have been equivalent
and one of the sets (x) ends, but the other one (y) does not end
(i.e. x is a strict prefix of y),
we used to get y < x, but with this changeset we are going to get x < y.

Example (in pseudocode) (with case-insensitive comparison):
we used to get {'A', 'b'} > {'a', 'B', 'c'},
now fixed to {'A', 'b'} < {'a', 'B', 'c'}.
This is similar to how std::lexicographical_compare behaves.

@onf-cla-manager
Copy link

onf-cla-manager bot commented Jul 13, 2022

Hi @DoctorNoobingstoneIPresume, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:

✒️ 👉 https://cla.opennetworking.org

After signing, make sure to add your Github user ID DoctorNoobingstoneIPresume to the agreement.

For more information or help:"
https://wiki.opennetworking.org/x/BgCUI

@DoctorNoobingstoneIPresume
Copy link
Contributor Author

DoctorNoobingstoneIPresume commented Jul 13, 2022

Perhaps we could replace the whole implementation (containing the loop) with just:

return std::lexicographical_compare
(
      data_map.begin (),   data_map.end (),
    a.data_map.begin (), a.data_map.end (),
    data_map.key_comp ()
);

@DoctorNoobingstoneIPresume DoctorNoobingstoneIPresume force-pushed the 220713_ordered_set_opCmp_01h branch 2 times, most recently from f797148 to 5609d69 Compare July 13, 2022 21:12
@mihaibudiu
Copy link
Contributor

Can you please try to run make cpplint prior to submitting the PR?

When all elements compared so far in sets `x` and `y` have been equivalent
and one of the sets (`x`) ends, but the other one (`y`) does not end
(i.e. `x` is a strict prefix of `y`),
we used to get `y < x`, but with this changeset we are going to get `x < y`.

Example (in pseudocode) (with case-insensitive comparison):
  we used to get `{'A', 'b'} > {'a', 'B', 'c'}`,
  now fixed to   `{'A', 'b'} < {'a', 'B', 'c'}`.
The new behaviour is similar to that of `std::lexicographical_compare`.
@DoctorNoobingstoneIPresume
Copy link
Contributor Author

Can you please try to run make cpplint prior to submitting the PR?

Thank you for review. I will try to do that. I dare suggest that maybe we remove cpplint ? :)

E.g.: Alternative implementation with std::lexicographical_compare (in comment above: #3431 (comment)) is never going to pass cpplint as it is, but I think it is clearer -- the additional whitespace makes obvious what is similar and what is different between the two lines.

@mihaibudiu
Copy link
Contributor

Having tools makes it much easier to enforce coding standards. Without tools like cpplint conversations can devolve into religious arguments. For example, I hate the defaults of tools like rustfmt, but I save a lot of time avoiding arguing about the right way to format Rust. In the end, time is more valuable.

@mihaibudiu mihaibudiu merged commit ad3c5ff into p4lang:main Jul 15, 2022
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.

2 participants