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

[WIP] feat: add the vfs in 'kclvm-vfs' #1154

Closed
wants to merge 1 commit into from

Conversation

zong-zhe
Copy link
Contributor

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

re #966

2. What is the scope of this PR (e.g. component or file name):

kclvm-vfs

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@zong-zhe zong-zhe added enhancement New feature or request arch labels Mar 22, 2024
@zong-zhe zong-zhe added this to the v0.9.0 Release milestone Mar 22, 2024
@zong-zhe zong-zhe requested a review from Peefy March 22, 2024 07:47
@zong-zhe zong-zhe self-assigned this Mar 22, 2024
@@ -153,6 +155,39 @@ pub fn parse_file_force_errors(filename: &str, code: Option<String>) -> Result<a
}
}

pub fn parse_file_with_session_vfs(
Copy link
Contributor

Choose a reason for hiding this comment

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

The vfs here should replace the SourceMap field in ParseSession, rather than being a separate parameter.

opts: LoadProgramOptions,
missing_pkgs: Vec<String>,
module_cache: Option<KCLModuleCache>,
file_graph: FileGraph,
compile_entries: Entries,
Copy link
Contributor

Choose a reason for hiding this comment

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

s/compile_entries/entries

}

impl Loader {
fn new_vfs(
Copy link
Contributor

Choose a reason for hiding this comment

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

The vfs here should replace the SourceMap field in ParseSession, rather than being a separate parameter and separate function new_vfs

@@ -307,30 +342,67 @@ pub fn load_program(
Loader::new(sess, paths, opts, module_cache).load_main()
}

pub fn load_program_vfs(
Copy link
Contributor

Choose a reason for hiding this comment

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

The vfs here should replace the SourceMap field in ParseSession, rather than being a separate parameter and a separate function.

@@ -0,0 +1,3 @@
pub mod entry;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest layering the VFS package, placing the general part in compiler_base, and incorporating KCL specific semantics through extension.

sf_inner: Arc<RwLock<kclvm_span::SourceFile>>,
}

pub struct SourceMapVfs {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming here feels strange, as SourceMap and VFS are actually similar concepts and it is not appropriate to juxtapose them.

}
}

impl VFS for SourceMapVfs {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do they appear simultaneously e.g., VFS and Vfs. Considering using the same name Vfs.


pub trait VFS {
// TODO: More actions to be get SourceFile directly
fn write(&self, path: String, contents: Option<Vec<u8>>) -> Result<()>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering a AsRef<Path> or AsRef<Utf8Path> parameter for the path variable.

@Peefy
Copy link
Contributor

Peefy commented Jun 12, 2024

Close due to inactive state

@Peefy Peefy closed this Jun 12, 2024
@zong-zhe zong-zhe deleted the add-kcl-vfs branch June 12, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants