python / cpython

The Python programming language
https://www.python.org
Other
62.61k stars 30.04k forks source link

simplify overlaps function in ipaddress.py #82516

Open 3a37abd6-e522-4049-958c-60aefc9639d0 opened 5 years ago

3a37abd6-e522-4049-958c-60aefc9639d0 commented 5 years ago
BPO 38335
Nosy @serhiy-storchaka, @s-sanjay
PRs
  • python/cpython#16519
  • Files
  • skip_broadcast_in.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['3.8', '3.7', 'library', '3.9'] title = 'simplify overlaps function in ipaddress.py' updated_at = user = 'https://github.com/s-sanjay' ``` bugs.python.org fields: ```python activity = actor = 'Sanjay' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'Sanjay' dependencies = [] files = ['48634'] hgrepos = [] issue_num = 38335 keywords = ['patch'] message_count = 3.0 messages = ['353676', '354004', '354005'] nosy_count = 2.0 nosy_names = ['serhiy.storchaka', 'Sanjay'] pr_nums = ['16519'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue38335' versions = ['Python 3.5', 'Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9'] ```

    3a37abd6-e522-4049-958c-60aefc9639d0 commented 5 years ago

    the current implementation of overlaps function tests either network or broadcast address is in other but we can skip checking broadcast address is in other because we anyway check if other.network_address in self

    without loss of generality if we assume self has smaller prefixlen than other then when self.broadcast_address in other then other.network_address *SHOULD* be in self but the reverse is not true

    so my first patch was to make the function logic simply do self.network_address in other or other.network_address in self

    but the current PR does a different change. We have introduced two new functions subnet_of and supernet_of

    for two networks A, B there are only three possibilities

    1. they don't overlap
    2. A is subnet of B
    3. B is subnet of A

    so we can reuse the existing function and just do return self.subnet_of(other) or self.supernet_of(other) the only thing is while overlaps() function returns false when we try to compare with a network or with diff version the other throws exception so I added a typecheck in the beginning.

    P.S the docstring is slightly convoluted for newcomers, based on the three possibilities it should say "Tell if self is supernet or subnet of other" because "partly contained" can also mean two ranges intersect which can never happen to network prefixes. I have not made that change but can make it.

    There are also some other issues I want to address but I want to do one at a time.

    serhiy-storchaka commented 4 years ago

    What is the benefit of your version?

    3a37abd6-e522-4049-958c-60aefc9639d0 commented 4 years ago

    the version in the patch is less code a or b or c or d becomes a or d

    the version in PR is using the subnet_of, supernet_of function because overlap means either a is subnet_of or supernet_of b. So was trying to consolidate the implementation of overlap from 2 to 1.