-
Notifications
You must be signed in to change notification settings - Fork 441
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
Make the new operator protected for some IR types. #4670
Conversation
dce0b88
to
53abeeb
Compare
Yes, it is still possible that |
e79584b
to
0032f0d
Compare
0032f0d
to
8af7050
Compare
@@ -81,7 +81,7 @@ const IR::Node *LowerExpressions::postorder(IR::Cast *expression) { | |||
} else if (destType->width_bits() < srcType->width_bits()) { | |||
// explicitly discard un needed bits from src | |||
auto one = new IR::Constant(srcType, 1); | |||
auto shift_value = new IR::Constant(new IR::Type_InfInt(), destType->width_bits()); | |||
auto shift_value = new IR::Constant(IR::Type_InfInt::get(), destType->width_bits()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this (and many other places) could just be rewritten as
auto shift_value = new IR::Constant(destType->width_bits());
the default type for constants created via the ctor that just takes an int
is InfInt. I don't think explicitly including the type adds anything -- its more of an obfuscation than a clarification.
@@ -120,6 +120,7 @@ abstract Type_Base : Type { | |||
class Type_Unknown : Type_Base { | |||
#nodbprint | |||
static Type_Unknown get(); | |||
static Type_Unknown get(const Util::SourceInfo &si); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there ever a case where we have an unknown type with a source info? I can't think of how it could occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think so. At least we do not store any source info for an unknown type when parsing. I mostly mirrored what the auto-generated constructors allow, so I do not think at can hurt at least.
ir/type.cpp
Outdated
const Type_Bits *Type_Bits::get(const Util::SourceInfo &si, int sz, bool isSigned) { | ||
auto *result = new IR::Type_Bits(si, sz, isSigned); | ||
if (sz < 0) { | ||
::error(ErrorType::ERR_INVALID, "%1%: Width of type cannot be negative", result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be BUG_CHECK
instead? Also, it seems we're creating result
here just for some message printing. I think we can just use sz
and create result only when all sanity checks pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why an error was chosen here but the comment
// Return a value that will not cause crashes later on
indicates this was deliberate?
I moved the result allocation down and changed the wording a bit.
f35ac86
to
6320d18
Compare
6320d18
to
273155f
Compare
273155f
to
cd5028c
Compare
Addresses some of the points raised in #4612. Note that #4612 is still required. Type pointers can still not be unique because of the source information associated with the type and the
clone
call.