Skip to content

Conversation

Mikachu2333
Copy link
Collaborator

问题描述

  1. 应当先审核 修复 chsrc_make_tmpfile 在win上错误处理不完善的问题 #298
  2. rust 换源提示有误 #281 的前置任务

方案与实现

  1. xy.h 增加辅助函数 xy_str_swap 用于释放旧指针的内存
  2. 改进众多函数的内存泄漏问题
  3. 需要额外注意的是,调整了xy_file_exist之前令人犯晕的变量命名

@Mikachu2333 Mikachu2333 mentioned this pull request Oct 6, 2025
@ccmywish
Copy link
Contributor

ccmywish commented Oct 6, 2025

这个 PR 有一部分和 #298 重复了 (创建临时文件)

@ccmywish
Copy link
Contributor

ccmywish commented Oct 6, 2025

本来设计的时候就没打算处理内存,如果现在要考虑内存处理,需要谨慎。

除了不确定能否完全达到修复所有内存泄漏的问题以外,就单从更改范围性来看,可能会引入问题,而这些问题现有的测试无法排除,很有可能 chsrc 本身的运行才会触发出一些问题。

而我们 v0.2.3 版本的发布时间已经接近: https://github.com/RubyMetric/chsrc/milestone/10

所以该 PR 考虑在 v0.2.3.1 开始引入。

@ccmywish
Copy link
Contributor

ccmywish commented Oct 6, 2025

单从代码上来看,质量很高 👍

v0.2.3 版本不考虑引入的直接原因是,没有时间窗口让用户测试暴露错误了,只能推到以后去测试。


另外,我依然希望,内存的释放尽量只停留在 xy.h 中,以便这个文件可以复用于其他地方和项目。

chsrc 本身,由于 recipe 会由不熟悉 C 语言的贡献者贡献,我觉得还是不用苛求。

@ccmywish ccmywish added this to the v0.2.4 milestone Oct 6, 2025
@Mikachu2333
Copy link
Collaborator Author

Mikachu2333 commented Oct 6, 2025

这个 PR 有一部分和 #298 重复了 (创建临时文件)

是的,不过没有冲突,本pr一开始就是在本地merge的基础上继续修改的。

原因也很简单,一开始我写的时候也没想过内存泄露的问题,结果改tmpfile的时候越改发现问题越多,于是就只把tmpfile的逻辑修了,内存泄露与句柄回收之类的问题全塞这里了。

v0.2.3 版本不考虑引入的直接原因是,没有时间窗口让用户测试暴露错误了,只能推到以后去测试。

可以理解,完全支持

另外,我依然希望,内存的释放尽量只停留在 xy.h 中,以便这个文件可以复用于其他地方和项目。

目前我只改了 core.c 和 xy.c 并且不打算扩大修正范围。修改 core的主要原因是用了太多的xy中的函数,只改xy对改善目前的泄露没啥帮助(约等于0甚至说)

记混了,core就漏了一点点,如果你认为需要回退core那也没啥大问题

chsrc 本身,由于 recipe 会由不熟悉 C 语言的贡献者贡献,我觉得还是不用苛求。

是的,我完全没有动各换源子项目的代码(我自己rust的也漏了一点其实),各菜谱漏一点都是可接受的、可预期的。只是我想在源头上尽量堵一堵,不然哪怕菜谱的代码圆满无缺,根子上还是四处漏风(

话说我怎么就不算“不熟悉c的贡献者”呢

果然还是得感谢ai,让我写我是真写不出来,但是让我看看逻辑,看看stack overflow的例子,对着cpp reference和winapi docs看看函数用的对不对还是能做到的(

话说winapi的文档中文真是纯屎,还得看英文

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR focuses on fixing memory leaks and resource management issues throughout the codebase. The primary goal is to ensure proper cleanup of dynamically allocated memory and file handles to prevent resource leaks.

Key changes:

  • Added xy_str_swap helper function for safe string pointer replacement with automatic memory cleanup
  • Fixed multiple memory leaks by adding proper free() calls for allocated strings and file handles
  • Improved variable naming in file existence checking functions for better clarity
  • Refactored string manipulation functions to prevent memory leaks

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/framework/core.c Added missing free() calls for temporary file paths and URL strings, fixed file handle leak in error path
lib/xy.h Added xy_str_swap() helper function, refactored string functions to use proper memory management, improved variable naming in file/directory existence functions

@ccmywish ccmywish requested a review from happy-game October 7, 2025 02:01
@ccmywish
Copy link
Contributor

ccmywish commented Oct 7, 2025

@happy-game

有空的话看一下。

重构 `xy_str_delete_prefix` 函数以避免不必要的内存分配和释放,
直接使用原字符串进行处理。同时修复 `xy_dir_exist` 函数中 system
调用后未释放临时命令字符串的内存泄漏问题。
将函数名从 `xy_str_swap` 更改为 `xy_str_replace`,以更准确地反映其实际功能。
该函数并非简单交换两个字符串,而是用新字符串替换旧字符串的内容。

此更改涉及多个文件中的调用点,包括路径处理和文件读取相关的逻辑。
@Mikachu2333 Mikachu2333 requested a review from happy-game October 7, 2025 10:59
@Mikachu2333
Copy link
Collaborator Author

@ccmywish 再问个事,为啥 xy_dir_exist 没调用 normalize而是自己又实现了一遍?这是有啥讲究么

@Mikachu2333 Mikachu2333 requested a review from Copilot October 7, 2025 11:06
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

@Mikachu2333 Mikachu2333 requested a review from Copilot October 7, 2025 11:23
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

@happy-game
Copy link
Collaborator

LGTM

@ccmywish
Copy link
Contributor

ccmywish commented Oct 7, 2025

@Mikachu2333

再问个事,为啥 xy_dir_exist 没调用 normalize而是自己又实现了一遍?这是有啥讲究么

因为 xy_normalize_path() 做的工作太多了,把两端空格都删除了。而 xy_dir_exist() 是纯粹的检查函数,接受的参数就应该是正确的目录字符串(如果你传了一个有问题的路径字符串,当然就应该是错误的)。所以,应该由程序员手动调用 xy_normalize_path(),而不是替程序员做该工作。

@Mikachu2333 Mikachu2333 requested a review from ccmywish October 7, 2025 14: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.

3 participants