Skip to content
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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Mar 31, 2025

Use this modifier to make sure we can not pass {} to constructors.

fruffy and others added 4 commits March 31, 2025 10:00
…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)
Copy link
Contributor

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.

Copy link
Collaborator Author

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;
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants