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.

Sign up for GitHub

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

Jump to bottom

[refactor] Re-export some functions called directly by ASTTransfomer.* as member of ASTBuilder #4034

Merged
merged 10 commits into from
Jan 20, 2022
Merged

[refactor] Re-export some functions called directly by ASTTransfomer.* as member of ASTBuilder #4034

merged 10 commits into from
Jan 20, 2022

Conversation

PGZXB
Copy link
Contributor

@PGZXB PGZXB commented Jan 15, 2022

Related issue = #4032

@vercel
Copy link

vercel bot commented Jan 15, 2022 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/taichi-api-docs-preview/taichi/wfgk9bUkFqUwDwjMXuCk8LHRLMEg
✅ Preview: https://taichi-git-fork-pgzxb-dev-export-529561-taichi-api-docs-preview.vercel.app

[Deployment for 24b45fa canceled]

@netlify
Copy link

netlify bot commented Jan 15, 2022 •
edited
Loading

✔️ Deploy Preview for docsite-preview ready!

🔨 Explore the source changes: 24b45fa

🔍 Inspect the deploy log: https://app.netlify.com/sites/docsite-preview/deploys/61e8d998ece6140007a65b91

😎 Browse the preview: https://deploy-preview-4034--docsite-preview.netlify.app

strongoier
strongoier reviewed Jan 17, 2022
View reviewed changes
Copy link
Contributor

@strongoier strongoier left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! The title needs to be modified because this PR only reflects the first step of the issue.

@lin-hitonami Is there any reason we make build_xxx static methods? I'm wondering whether we can put ast_builder as a member variable of ASTTransformer. Another option might be to put it under ctx, but I'm not sure if that's proper.

taichi/program/kernel.h Outdated Show resolved Hide resolved
python/taichi/lang/ast/ast_transformer.py Outdated Show resolved Hide resolved
@PGZXB PGZXB changed the title [refactor] Export ASTBuilder to remove some dependencies on global accessing [refactor] Re-export some functions called directly by taichi.lang.ast.ASTTransfomer.* as member of ASTBuilder. Jan 17, 2022
@PGZXB PGZXB changed the title [refactor] Re-export some functions called directly by taichi.lang.ast.ASTTransfomer.* as member of ASTBuilder. [refactor] Re-export some functions called directly by ASTTransfomer.* as member of ASTBuilder. Jan 17, 2022
@PGZXB PGZXB changed the title [refactor] Re-export some functions called directly by ASTTransfomer.* as member of ASTBuilder. [refactor] Re-export some functions called directly by ASTTransfomer.* as member of ASTBuilder Jan 17, 2022
@PGZXB @strongoier
Remove unnecessary print statement
c28fa12 
Co-authored-by: Yi Xu <xy_xuyi@foxmail.com>
@PGZXB @strongoier
Set
b4d8338 
Co-authored-by: Yi Xu <xy_xuyi@foxmail.com>
@lin-hitonami
Copy link
Contributor

Looks great! The title needs to be modified because this PR only reflects the first step of the issue.

@lin-hitonami Is there any reason we make build_xxx static methods? I'm wondering whether we can put ast_builder as a member variable of ASTTransformer. Another option might be to put it under ctx, but I'm not sure if that's proper.

Maybe putting it under ctx makes the code cleaner? In fact everything inside ctx can be moved to ASTTransformer if we don't make build_xxx static method. cc @ailzhang

@ailzhang
Copy link
Contributor

ailzhang commented Jan 18, 2022 •
edited
Loading

Maybe putting it under ctx makes the code cleaner? In fact everything inside ctx can be moved to ASTTransformer if we don't make build_xxx static method. cc @ailzhang

Yea makes sense, how about putting it in ctx first in this PR? I also don't see any blocker for merging ctx into ASTTransformer but it better stays as a separate PR ;)

PGZXB and others added 2 commits January 19, 2022 11:40
@PGZXB @strongoier
Remove unnecessary line
f9196b8 
Co-authored-by: Yi Xu <xy_xuyi@foxmail.com>
@PGZXB
Merge branch 'dev-export-ASTBuilder' of github.com:PGZXB/taichi into …
506e322 
…dev-export-ASTBuilder
@PGZXB PGZXB closed this Jan 19, 2022
@PGZXB PGZXB reopened this Jan 19, 2022
strongoier
strongoier approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@strongoier strongoier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

taichi/python/export_lang.cpp Show resolved Hide resolved
taichi/program/program.h Outdated Show resolved Hide resolved
taichi/python/export_lang.cpp Show resolved Hide resolved
@PGZXB @strongoier
Remove unnecessary comment
24b45fa 
Co-authored-by: Yi Xu <xy_xuyi@foxmail.com>
@strongoier strongoier merged commit 5534280 into taichi-dev:master Jan 20, 2022
@PGZXB PGZXB deleted the dev-export-ASTBuilder branch September 19, 2022 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@strongoier strongoier strongoier approved these changes

@lin-hitonami lin-hitonami Awaiting requested review from lin-hitonami

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PGZXB @lin-hitonami @ailzhang @strongoier