-
Notifications
You must be signed in to change notification settings - Fork 461
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
[Generator] Add an explicit modifier to the IR generator. #5208
base: main
Are you sure you want to change the base?
Conversation
…ct pass. (#5193) Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
…lpointer for Annotations. Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
@@ -252,7 +252,7 @@ class Annotation { | |||
inline Annotation(Util::SourceInfo si, ID n, const IndexedVector<NamedExpression> &kv, | |||
bool structured = false) | |||
: Node(si), name(n), body(kv), structured(structured) {} | |||
inline Annotation(ID n, const Expression *a, bool structured = false) | |||
inline explicit Annotation(ID n, const Expression *a, bool structured = false) |
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.
It doesn't seem like the explicit
does anything useful here -- it just disallows things like
IR::Annotation annot = { some_id, some_expression };
which doesn't seem too useful.
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.
Right, I started out with this approach and then realized the Expression constructor must be made explicit. Figured it is still useful to add this modifier to the generator.
I haven't made the necessary changes to the generator yet to support this. There is still a bit of work to do.
// FIXME: We shouldn't need this delete. | ||
// Ideally, we only accept references to expressions. | ||
void addOrReplaceAnnotation(cstring name, std::nullptr_t expr, bool structured = false) = delete; | ||
void addAnnotation(cstring name, std::nullptr_t expr, bool structured = false) = delete; |
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.
While calling these with an explicit nullptr
doesn't make sense, does this do anything for the {}
case? I think what we need is something like:
inline void addOrReplaceAnnotation(cstring name, std::initializer_list<const Expression *> elist, bool structured = false) {
Annotations::addOrReplace(getAnnotations(), new Annotation(name, elist, structured));
}
or perhaps add Annotations::addOrReplace(Vector<Annotation> &, cstring, std::initializer_list<const Expression *>, bool)
and call that.
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.
Deleting this overload works because the compiler will throw an error that the call is ambiguous. It is not pretty, but doesn't break the interface.
Probably the right approach is to make everything pass-by-reference.
Use this modifier to make sure we can not pass
{}
to constructors.