-
Notifications
You must be signed in to change notification settings - Fork 127
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(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner #71
Conversation
4e0c496
to
e86954b
Compare
…cuting in kclvm/lib.rs into kclvm-runner The assembler and linker of the current version of the compiler are not separately packaged. In order to support the reuse of modules such as dylibs generating,linking and executing, This modification separates dylibs generating,and encapsulate them individually into KclvmAssembler and KclvmLinker. Encapsulate dylibs generating, linking and executing into kclvm-runner/assembler.rs and kclvm-runner/linker.rs. Add struct "KclvmAssembler" in kclvm-runner/assembler.rs to provide method "gen_dylibs" for dylibs generating. Add struct "KclvmLinker" in kclvm-runner/linker.rs to provide method "link_all_dylibs" for dylib linking. Add method "execute" in kclvm-runner/lib.rs to encapsulate dylibs generating(gen_dylib), dylib linking(link_all_dylib) and running(runner.run) together. fix #67
e86954b
to
f8e2142
Compare
fa3b156
to
18b5366
Compare
Pls put benchmark stats into the PR info. |
18b5366
to
4c272db
Compare
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.
As a refactor PR, it would be good to have some preparation for future backend target abstractions.
4c272db
to
99da07b
Compare
af8d1c9
to
dfc6b70
Compare
Do not hardcode 'dylib' in all naming. In fact, it may be .ddl, .dylib, .so on different platforms. I prefer to change all 'dylib's to 'lib's. |
This refactoring should preferably do some preparation for the subsequent target abstraction refactoring. For example, dylib and llvm were all hardcoded as the only impl way, you may need to change them into variables first, and then the next refactoring will replace these variables through traits or other dynamic methods to make it a target abstraction with a set of impls. At the same time, do not hardcode implementation keywords like 'dylib', 'llvm' in the comments and docstrings. |
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.
You'd better lint code by using cargo clippy
in the runner
crate.
dfc6b70
to
838f474
Compare
dcb5ab1
to
0fe6ac2
Compare
8b762e4
to
2c21d67
Compare
2c21d67
to
efe821a
Compare
958cb57
to
c589e87
Compare
597789b
to
debf405
Compare
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.
LGTM
…ting into kclvm-runner. 1. Encapsulate generating of libs in "kclvm/src/lib.rs" and "kclvm/src/main.rs" into "kclvm/runner/assembler.rs". 2. Encapsulate linking of libs in "kclvm/src/lib.rs" and "kclvm/src/main.rs" into "kclvm/runner/linker.rs" 3. Encapsulate executing of libs in "kclvm/src/lib.rs" and "kclvm/src/main.rs" into "kclvm/runner/lib.rs". 4. A timer is added during the concurrent multi-file compilation to prevent KCLVM locked due to child thread panic. fix #67 #106 #82
debf405
to
6409a6d
Compare
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.
LGTM
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.
LGTM
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
fix #67 and #82
2. What is the scope of this PR (e.g. component or file name):
kclvm-runner/runner.rs
kclvm-runner/lib.rs
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:
Add test cases in kclvm-runner/src/tests.rs and kclvm-runner/test_datas.
Add bench in kclvm-runner/benches/bench_runner.rs
Benchmark stats
1. KCL Test Case
2. Bench Stats (Compare with before refactoring)
2.1 CLI Report
2.2 HTML Report
6. Release note
Please refer to Release Notes Language Style Guide to write a quality release note.