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

[refactor] Move c_quoted into taichi/utils/ #1376

Merged
merged 2 commits into from
Jul 3, 2020
Merged

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Jul 2, 2020

Moving this to a common utils so that IR printer can also use it.

@archibate I don't have a way to test the C backend on Mac. Could you confirm this doesn't break?

Related issue = #1350 (comment)

[Click here for the format server]


@k-ye k-ye requested a review from archibate July 2, 2020 12:21
@codecov
Copy link

codecov bot commented Jul 2, 2020

Codecov Report

Merging #1376 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1376      +/-   ##
==========================================
- Coverage   85.65%   85.60%   -0.06%     
==========================================
  Files          19       19              
  Lines        3373     3375       +2     
  Branches      624      625       +1     
==========================================
  Hits         2889     2889              
- Misses        354      356       +2     
  Partials      130      130              
Impacted Files Coverage Δ
python/taichi/lang/ast_checker.py 72.22% <0.00%> (-2.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b181a2...e9cecb4. Read the comment docs.

Copy link
Collaborator

@archibate archibate left a comment

Choose a reason for hiding this comment

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

Compiling & running happily on my end! Thanks!
Btw, would you help me set up C backend on OS X after #1354 gets merged?

@archibate
Copy link
Collaborator

But, how about to move this to lang_util.h and lang_util.cpp? I'm afraid adding a new C++ file just for this may increase compile time.. Or we do lang_util.cpp refactor IAPR?

@k-ye
Copy link
Member Author

k-ye commented Jul 2, 2020

But, how about to move this to lang_util.h and lang_util.cpp? I'm afraid adding a new C++ file just for this may increase compile time..

Is there any evidence this might hurt compilation time? I'd suggest to keep lang_util as simple as possible, because it has been included by almost all the files, in order to have this TLANG_NAMESPACE_BEGIN|END macros?

@k-ye
Copy link
Member Author

k-ye commented Jul 2, 2020

Btw, would you help me set up C backend on OS X after #1354 gets merged?

Sure. I just did a local test with C backend enabled. Didn't encounter compilation error at least...

@archibate archibate added the LGTM label Jul 3, 2020
@archibate
Copy link
Collaborator

lang_util

Sounds reasonable, thank for the hard work!

Didn't encounter compilation error

Would you run

ti.init(ti.cc)

@ti.kernel
def hello():
  print('Hello, world!')

hello()

to confirm? TTYM in the C backend issue. Let's merge this for now.

@archibate archibate merged commit 36c2078 into taichi-dev:master Jul 3, 2020
@FantasyVR FantasyVR mentioned this pull request Jul 4, 2020
@k-ye k-ye deleted the quote branch July 9, 2020 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants