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

Better Handing of Integer Literals #122

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@idrisr
Contributor

idrisr commented Oct 15, 2015

This is a follow up to #119. Now evaluateIntegerExpression() will accept an integer literal of any base and return the base10 value. Also made code more consistent to call evaluateIntegerExpression() where possible instead of int(evaluateExpression())

@@ -63,7 +63,7 @@ def evaluateIntegerExpression(expression, printErrors=True):
output = output[2:]
elif output.startswith('\\'): # Or as \0 (Dec)
output = output[1:]
return int(output, 16)
return int(output, 0)

This comment has been minimized.

@erydo

erydo Oct 25, 2015

Because the stripped prefix indicates the base of the literal, it seems like it would make more sense to preserve that information rather than asking int() to attempt to automatically detect.

For example, if the output were \x20 (decimal 32) and the prefix were stripped, int() would assume 20 was in base 10.

Suggestion:

if output.startswith('\\x'): # Booleans may display as \x01 (Hex)
    output = output[2:]
    base = 16
elif output.startswith('\\'): # Or as \0 (Dec)
    output = output[1:]
    base = 10
else:
    base = 10
  return int(output, base)

Three additional comments, from my own unfamiliarity:

  1. Because the expression given to LLDB is wrapped in (int)(…) above, doesn't LLDB always return that value in decimal? It isn't attempting to render a pointer type.
  2. The 0 prefix is usually used to indicate octal, not decimal as indicated in that comment. Is that different here?
  3. To accommodate cases of e.g. a ptrdiff_t which may be longer than an int, should the above use (long long) rather than (int)?
@idrisr

This comment has been minimized.

Contributor

idrisr commented Oct 28, 2015

  1. I believe that LLDB will return the expression cast to int in decimal, though I am not certain enough on this to say this is true in all cases.
  2. The 0 prefix does indicate a octal usually, but not for this python command. The second argument is the numeric value of the base you want, so if you wanted octal, the 2nd argument would be 8. I'm including the int() docstring for Python below.
In [1]: int?
Docstring:
int(x=0) -> int or long
int(x, base=10) -> int or long

Convert a number or string to an integer, or return 0 if no arguments
are given.  If x is floating point, the conversion truncates towards zero.
If x is outside the integer range, the function returns a long instead.

If x is not a number or if base is given, then x must be a string or
Unicode object representing an integer literal in the given base.  The
literal can be preceded by '+' or '-' and be surrounded by whitespace.
The base defaults to 10.  Valid bases are 0 and 2-36.  Base 0 means to
interpret the base from the string as an integer literal.
  1. That's a good point. ptrdiff_t can be larger than an int. https://www.gnu.org/software/libc/manual/html_node/Important-Data-Types.html
@erydo

This comment has been minimized.

erydo commented Oct 28, 2015

@idrisr — Re 0 for octal, I was not referring to the argument to int. I was referring to the comment in the line:

elif output.startswith('\\'): # Or as \0 (Dec)

Which appears to erroneously interpret LLDB output with a leading \0 as representing decimal when it likely should represent octal and use base = 8 instead in the suggestion above.

@idrisr

This comment has been minimized.

Contributor

idrisr commented Nov 1, 2015

Yeah that comment is misleading. '0' indicates an octal string literal.

@kastiglione

This comment has been minimized.

Collaborator

kastiglione commented Nov 6, 2015

So is it correct that the remaining changes here are:

  • Preserve base information from lldb output
  • Fix or remove incorrect "Dec" comment when actually handle octal
  • Cast to long long
@kastiglione

This comment has been minimized.

Collaborator

kastiglione commented Nov 6, 2015

Thanks @idrisr and @erydo for quality on this!

@ghost ghost added the CLA Signed label Jul 12, 2016

@idrisr idrisr closed this Jul 19, 2018

@idrisr

This comment has been minimized.

Contributor

idrisr commented Jul 19, 2018

This PR can be closed at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment