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

migrating check_and_promote() and Expr::InitVal() to use IntrusivePtr's #1624

Closed
vpax opened this issue Jun 25, 2021 · 1 comment · Fixed by #1918
Closed

migrating check_and_promote() and Expr::InitVal() to use IntrusivePtr's #1624

vpax opened this issue Jun 25, 2021 · 1 comment · Fixed by #1918

Comments

@vpax
Copy link
Contributor

vpax commented Jun 25, 2021

check_and_promote() is a function declared in Val.h that currently takes a ValPtr argument and a const Type* argument (as well as a bool and an optional Location* argument). @timwoj flagged in his review of #1578 that it would be more tidy if the second argument were const TypePtr&. I looked into making that change (which I agree with) and found that doing so is fairly involved, since Expr::InitVal() itself takes a const Type* that it then passes to check_and_promote(), and Expr::InitVal() is called in a bunch of places.

This issue is meant to flag the general desirability of these changes, but at a low priority given it's likely a fair amount of low-level grunt work to actually implement.

@timwoj
Copy link
Member

timwoj commented Dec 13, 2021

I looked at this briefly and there's honestly a ton more we could do here with migrating to use IntrusivePtr, even just in the Expr code. For example, all of these methods:

extern bool check_and_promote_exprs(ListExpr* elements, TypeList* types);
extern bool check_and_promote_args(ListExpr* args, const RecordType* types);
extern bool check_and_promote_exprs_to_type(ListExpr* elements, TypePtr type);

There's also two copies of the is_vector() method, one which takes a raw pointer and one which takes an IntrusivePtr. The former can be removed.

Doing all of the migration is honestly a project unto itself, and we should try to do all of it at once instead of piece-by-piece.

@timwoj timwoj added this to the 5.0.0 milestone Jan 4, 2022
timwoj added a commit that referenced this issue Jan 7, 2022
…promote'

* origin/topic/timw/1624-migrate-checkandpromote:
  GH-1624: Migrate check_and_promote and a few Expr methods to IntrusivePtr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants