refactor IntegerExp #3495
refactor IntegerExp #3495
Conversation
| } | ||
|
|
||
| IntegerExp::IntegerExp(dinteger_t value) | ||
| : Expression(Loc(), TOKint64, sizeof(IntegerExp)) | ||
| { | ||
| this->type = Type::tint32; | ||
| this->value = value; | ||
| this->value = (d_int32) value; |
There was a problem hiding this comment.
No - the original didn't check, either.
There was a problem hiding this comment.
No, because the 'normalization' is not an error. To simplify the logic, computations are done on a dinteger_t, then 'normalized' down to match the type. This is the way it has always worked. The method is not incorrect, it just moves all the normalization to IntegerExp itself rather than spreading it around.
There was a problem hiding this comment.
Why is hiding the narrowing cast inside the constructor a good thing? Is this constructor used for anything other than creating small int constants? Is there correct code that doesn't explicitly set a type, but is still ok with out-of-range dinteger_t values being truncated?
There was a problem hiding this comment.
It's not hiding it. All normalization is done by IntegerExp, not by the caller. The 'truncation' is normal 2-s complement arithmetic, and this arithmetic happens, especially during constant folding. In order to avoid a complex mess of code, all calculations are done to dinteger_t precision and then truncated.
It's always worked this way, and it is not a bug, and it is not hiding bugs.
There was a problem hiding this comment.
If this was being done during constant folding, then the constructor with a type would be being used, no?
There was a problem hiding this comment.
That constructor is a convenience function because most of the time the type is int. If C++ had forwarding constructors I'd have written it to simply forward to the other one.
|
lgtm but value should be private now that it's nicely encapsulated |
Eliminates duplicate code, and some dead code. Simplifies the logic. Makes assumptions explicit with assert. Steps towards making IntegerExp copy-on-write. No semantic changes.