current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite

I am looking for suggestions of improving this function to check whether a memory address is appropriate or not.

def isweird(addr, cpu_name):
    """Check if a memory address is weird
    Args:
        addr (str): a memory address
        cpu_name (str): cpu name
    Returns:
        bool: True if the address is weird
    """
    if not isinstance(addr, six.string_types):
        raise Exception('The memory address must be a string.')

    if addr == '0x0':
        return True

    addr = addr.lower()

    # Strip leading zeroes
    addr = addr[2:].lstrip('0')

    if utils.is64(cpu_name):
        if len(addr) <= 8:
            val = int(addr, 16)
            if val <= (1 << 16):  # val <= 0xffff (ie: first 64k)
                return True
        elif addr.startswith('ffffffff'):
            addr = addr[8:]  # 8 == len('ffffffff')
            val = int(addr, 16)
            if val >= ((1 << 32) - (1 << 16)):  # val >= 0xffffffffffff0000 (ie: last 64k)
                return True
    else:
        val = int(addr, 16)
        if val <= 1 << 16:  # val <= 0xffff (ie: first 64k)
            return True
        if val >= ((1 << 32) - (1 << 16)):  # val >= 0xffff0000 (ie: last 64k)
            return True
    return False
share|improve this question
up vote 5 down vote accepted

In this code:

if len(addr) <= 8:
    val = int(addr, 16)
    if val <= (1 << 16):  # val <= 0xffff (ie: first 64k)
        return True
elif addr.startswith('ffffffff'):
    addr = addr[8:]  # 8 == len('ffffffff')
    val = int(addr, 16)
    if val >= ((1 << 32) - (1 << 16)):  # val >= 0xffffffffffff0000 (ie: last 64k)
        return True

You can replace the inner if statements with return statements:

if len(addr) <= 8:
    val = int(addr, 16)
    return val <= (1 << 16)  # val <= 0xffff (ie: first 64k)
elif addr.startswith('ffffffff'):
    addr = addr[8:]  # 8 == len('ffffffff')
    val = int(addr, 16)
    return val >= ((1 << 32) - (1 << 16))  # val >= 0xffffffffffff0000 (ie: last 64k)

This is possible because if these conditions are false, the execution will reach the return False at the end of the function. The reason to rewrite this way is so that the reader doesn't have to follow the execution path until the end of the function, he can know on these lines that no further statements will be executed, the return value is already decided at these points.

Some of the comments are incorrect and misleading:

if val <= (1 << 16):  # val <= 0xffff (ie: first 64k)

if val >= ((1 << 32) - (1 << 16)):  # val >= 0xffffffffffff0000 (ie: last 64k)

1 << 16 is 0x10000, not 0xffff.

(1 << 32) - (1 << 16) is 0xffff0000, not 0xffffffffffff0000.

Following the naming conventions of Python, the function isweird should be named is_weird instead.

share|improve this answer
up vote 4 down vote

Don't use binary operators when you can use numbers. Take:

if val <= (1 << 16):  # val <= 0xffff (ie: first 64k)

This can just be:

if val <= 0xffff:

This allows you to remove the comments, whilst keeping clarity for users to understand what numbers you are using.

share|improve this answer
    
"boolean operators" I guess you meant "binary operators" – Caridorc Aug 31 '16 at 18:39
1  
@Caridorc Yes, thank you, silly me, ): – Peilonrayz Aug 31 '16 at 18:58
    
Minor correction, 1<<16 == 0x10000 – user650881 Jan 11 at 5:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.