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

fix: 修复部分函数使用 ege_transform_matrix 参数做变换时崩溃的问题 #234

Merged
merged 5 commits into from
Oct 4, 2024

Conversation

yixy-only
Copy link
Collaborator

No description provided.

@yixy-only yixy-only self-assigned this Oct 3, 2024
src/gdi_conv.cpp Outdated
* 将ege_transform_matrix 转换为 Gdiplus::Matrix
* matrix 参数不为 NULL 时返回 Matrix 对象指针,否则返回 NULL
*/
Gdiplus::Matrix* matrixConvert(const ege_transform_matrix* matrix)
Copy link
Owner

Choose a reason for hiding this comment

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

这种设计方式不太好吧, 返回一个需要delete的对象, 直接传入引用, 然后 return 对象是不是更好, 也不用处理 NULL 的情况了。

Copy link
Owner

Choose a reason for hiding this comment

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

就算是按 C 风格的设计模式, 应该也是把输出一起传入才对? 比如 itoa 啥的...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这种设计方式不太好吧, 返回一个需要delete的对象, 直接传入引用, 然后 return 对象是不是更好, 也不用处理 NULL 的情况了。

@wysaid OK,可以改成引用的方式。当时这么设计主要是考虑两点:

  • Gdiplus::Matrix 禁止赋值,返回不了对象,只能以指针的形式返回。
  • GDI+ 涉及变换的函数都对空指针特殊处理,不进行变换。如果改成对象,就需要改为传入单位矩阵,需要并且有额外计算。实际使用时代码处理也会更复杂一些。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

因为是 C 形式的接口,ege_transform_matrix 是指针参数,所以空指针判断操作避免不了

Copy link
Owner

Choose a reason for hiding this comment

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

主要还是要保持的 c++ 版本比较低, 不然你可以用 std::optional 之类的 来防止 new, 也能很好利用 RVO/NRVO 啥的....
要封装成函数确实麻烦... 如果封装成宏定义, 倒是简单很多...
我看你还单独为这个函数单独弄了个头文件....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

主要还是要保持的 c++ 版本比较低, 不然你可以用 std::optional 之类的 来防止 new, 也能很好利用 RVO/NRVO 啥的.... 要封装成函数确实麻烦... 如果封装成宏定义, 倒是简单很多... 我看你还单独为这个函数单独弄了个头文件....

@wysaid 因为后续可能还会加不少类似的类型转换,就单独建一个源文件了,转换操作统一放里面

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wysaid 改好了,看看这样可以不

@yixy-only yixy-only requested a review from wysaid October 4, 2024 06:32
src/gdi_conv.h Outdated
namespace ege
{
/* 矩阵类型转换 */
void matrixConvert(const ege_transform_matrix* from, Gdiplus::Matrix& to);
Copy link
Owner

Choose a reason for hiding this comment

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

看了下你使用的地方, 其实这里的 from 也可以传引用? 这样你的函数的 body 就简单了, 不需要写 if/else 了
这个函数没必要接受一个莫名其妙的 NULL 进来, 没啥意义的嘛

@yixy-only yixy-only requested a review from wysaid October 4, 2024 14:51
@wysaid wysaid merged commit 2527cf2 into wysaid:master Oct 4, 2024
1 check passed
@yixy-only yixy-only deleted the fix_path_transform_matrix branch October 5, 2024 00: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