Issue 8807 - Better diagnostic for literals in case statements. #1514

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Jan 20, 2013

http://d.puremagic.com/issues/show_bug.cgi?id=8807

This will print float literals as floating-point instead of rounding them down to integrals, which confuses the user.

Also, exp = new IntegerExp(0); was changed to exp = new ErrorExp();, otherwise multiple errors in switch statements would introduce multiple switch 0: cases and this would error about duplicate case statements.

- error("case must be a string or an integral constant, not %s", exp->toChars());
- exp = new IntegerExp(0);
+ OutBuffer buf;
+ if (exp->op == TOKfloat32 || exp->op == TOKfloat64 || exp->op == TOKfloat80)

This comment has been minimized.

Show comment Hide comment
@9rnsr

9rnsr Jan 20, 2013

Member

if (exp->type->isreal()) ?

@9rnsr

9rnsr Jan 20, 2013

Member

if (exp->type->isreal()) ?

This comment has been minimized.

Show comment Hide comment
@yebblies

yebblies Jan 20, 2013

Member

if (exp->isConst() && exp->type->isreal()) ?

@yebblies

yebblies Jan 20, 2013

Member

if (exp->isConst() && exp->type->isreal()) ?

This comment has been minimized.

Show comment Hide comment
@9rnsr

9rnsr Jan 20, 2013

Member

exp is already interpreted before a few lines.

@9rnsr

9rnsr Jan 20, 2013

Member

exp is already interpreted before a few lines.

This comment has been minimized.

Show comment Hide comment
@9rnsr

9rnsr Jan 20, 2013

Member

Ah..., but certainly, it does not guarantee that exp is a constant. So adding exp->isConst() might be better.

@9rnsr

9rnsr Jan 20, 2013

Member

Ah..., but certainly, it does not guarantee that exp is a constant. So adding exp->isConst() might be better.

This comment has been minimized.

Show comment Hide comment
@yebblies

yebblies Jan 20, 2013

Member

Even worse, if it's a variable it won't try to interpret.

@yebblies

yebblies Jan 20, 2013

Member

Even worse, if it's a variable it won't try to interpret.

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jan 20, 2013

So isConst is used as a defacto isLiteral? Because I've been trying to find the latter for a while now.

@ghost

ghost Jan 20, 2013

So isConst is used as a defacto isLiteral? Because I've been trying to find the latter for a while now.

This comment has been minimized.

Show comment Hide comment
@yebblies

yebblies Jan 20, 2013

Member

Yes. I suspect isConst predates D2's const.

@yebblies

yebblies Jan 20, 2013

Member

Yes. I suspect isConst predates D2's const.

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jan 20, 2013

Thanks. It's going to be one of those things to go to an updated DMDFE hacking wiki entry.

@ghost

ghost Jan 20, 2013

Thanks. It's going to be one of those things to go to an updated DMDFE hacking wiki entry.

+ else
+ buf.printf("%s", exp->toChars());
+ buf.writebyte(0);
+ error("case must be a string or an integral constant, not %s", (char *)buf.extractData());

This comment has been minimized.

Show comment Hide comment
@9rnsr

9rnsr Jan 20, 2013

Member

buf.extractData() returns char *, so cast is not need.

@9rnsr

9rnsr Jan 20, 2013

Member

buf.extractData() returns char *, so cast is not need.

@yebblies

This comment has been minimized.

Show comment Hide comment
@yebblies

yebblies Jan 20, 2013

Member

Why doesn't FloatExp::toChars print out the values correctly? Is that the real problem?

Member

yebblies commented Jan 20, 2013

Why doesn't FloatExp::toChars print out the values correctly? Is that the real problem?

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jan 20, 2013

Why doesn't FloatExp::toChars print out the values correctly? Is that the real problem?

It reduces 1.0 to 1, leaving the impression that the compiler interpreted the literal as an integral, while at the same time complaining that it's not an integral.

ghost commented Jan 20, 2013

Why doesn't FloatExp::toChars print out the values correctly? Is that the real problem?

It reduces 1.0 to 1, leaving the impression that the compiler interpreted the literal as an integral, while at the same time complaining that it's not an integral.

@yebblies

This comment has been minimized.

Show comment Hide comment
@yebblies

yebblies Jan 20, 2013

Member

It reduces 1.0 to 1, leaving the impression that the compiler interpreted the literal as an integral, while at the same time complaining that it's not an integral.

Maybe this should be changed in the default representation then. I doubt this is the only place where this comes up.

Member

yebblies commented Jan 20, 2013

It reduces 1.0 to 1, leaving the impression that the compiler interpreted the literal as an integral, while at the same time complaining that it's not an integral.

Maybe this should be changed in the default representation then. I doubt this is the only place where this comes up.

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jan 20, 2013

Maybe this should be changed in the default representation then. I doubt this is the only place where this comes up.

Yeah, I was never a fan of it. Changing this would avoid having to write special-cases like in this pull. However I have no idea how ld_sprint works, which is used in RealExp::toChars on win32. If someone wants to have a go at sometime later I can close this pull right now.

ghost commented Jan 20, 2013

Maybe this should be changed in the default representation then. I doubt this is the only place where this comes up.

Yeah, I was never a fan of it. Changing this would avoid having to write special-cases like in this pull. However I have no idea how ld_sprint works, which is used in RealExp::toChars on win32. If someone wants to have a go at sometime later I can close this pull right now.

@yebblies

This comment has been minimized.

Show comment Hide comment
@yebblies

yebblies Jan 20, 2013

Member
if (((real_t)(dinteger_t)value) == value)
  strcat(buffer, ".0");
Member

yebblies commented Jan 20, 2013

if (((real_t)(dinteger_t)value) == value)
  strcat(buffer, ".0");
@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jan 20, 2013

@yebblies: That seems to work. But I can't tell whether this might overflow the buffer, I don't understand the magic numbers for the size:

char buffer[sizeof(value) * 3 + 8 + 1 + 1];

ghost commented Jan 20, 2013

@yebblies: That seems to work. But I can't tell whether this might overflow the buffer, I don't understand the magic numbers for the size:

char buffer[sizeof(value) * 3 + 8 + 1 + 1];
@yebblies

This comment has been minimized.

Show comment Hide comment
@yebblies

yebblies Jan 20, 2013

Member

I don't understand the magic numbers for the size

Neither do I, it's from some ancient formula to calculate the stack space required to print different sized integers. Just change it to char buffer[256] or something.

Member

yebblies commented Jan 20, 2013

I don't understand the magic numbers for the size

Neither do I, it's from some ancient formula to calculate the stack space required to print different sized integers. Just change it to char buffer[256] or something.

@ghost ghost closed this Jan 20, 2013

This issue was closed.

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