-
Notifications
You must be signed in to change notification settings - Fork 74
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
feat: Added a toggle enableDisplayCustomTypeInfo aimed at further enhancing performance #53
Merged
CppCXY
merged 8 commits into
EmmyLua:master
from
zhangjiequan:feat/extend_all_lua_types_for_pr
Feb 23, 2024
Merged
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
7ca4b51
feat: Added a toggle enableDisplayCustomTypeInfo aimed at further enh…
zhangjiequan c596cf2
Merge branch 'master' into feat/extend_all_lua_types_for_pr
zhangjiequan 0832024
style: Update method names to follow PascalCase convention
zhangjiequan 8f1d78d
Revert "style: Update method names to follow PascalCase convention"
zhangjiequan 1e94fc1
Revert "feat: Added a toggle enableDisplayCustomTypeInfo aimed at fur…
zhangjiequan 9b97cec
feat: Added a bool displayCustomTypeInfo aimed at further enhancing p…
zhangjiequan fb24157
style: Replace spaces with tabs for consistent indentation across files
zhangjiequan e8bfb0e
style: Replace spaces with tabs for consistent indentation across files
zhangjiequan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
这里的所有判断可以只保留registeredTypes.test(type), 因为lua_type的返回值是确定的不会受外部影响增加,所以边界判断毫无意义
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.
可能的返回值-1
首先,关于您提到的lua_type的返回值范围的问题,它是否可能返回-1?
有定义:#define LUA_TNONE (-1),
这是在对应index没有元素时会发生的,因此确实需要考虑这种情况?
仅判断registeredTypes.set的可行性
如果我们确信lua_type的返回值严格在-1至8的范围内,我们确实可以考虑简化判断逻辑以提升性能。
然而,这种性能优化会以牺牲代码可读性为代价。这个tradeoff是否值得,我认为是有待商榷的。
以下是 仅判断registeredTypes.set 方案的示意代码:
性能比较:单一判断 vs 组合判断
当
enableDisplayCustomTypeInfo
被禁用且没有注册任何类型时,我们比较仅使用
registeredTypes.test(type+1)
与enableDisplayCustomTypeInfo && registeredTypes.test(type+1)
两种方式的性能。针对后者,根据短路求值的特性,只需要考虑
enableDisplayCustomTypeInfo
的开销。即我们对比两者的话,需要对比
registeredTypes.test(type+1)
与enableDisplayCustomTypeInfo
,显然后者能提供更好的性能,因为它避免了不必要的类型检查,只需要进行最直接的布尔值检查。而前者的类型检查,涉及函数调用等开销,性能稍差。所以我认为保留
enableDisplayCustomTypeInfo
作为一个配置选项是有价值的,它为性能优化提供了额外的灵活性。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.
这里的性能其实没那么重要, 主要是写的太冗长了, 另外就算你要保留enableDisplayCustomTypeInfo的设计, 那么也不应该提供独立的函数去开启这种功能, 因为registerType 和enable这两个函数一定是一起调用的, 所以可以直接在registerType内设置enableDisplayCustomTypeInfo = true
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.
我充分理解并尊重你的看法。
影响尽可能的要小
、性能要足够的高
(你在Supports customizing the display of all data types in Lua on the project side(支持在项目侧自定义Lua中所有类型数据的展示方式) by zhangjiequan · Pull Request #51 · EmmyLua/EmmyLuaDebugger中提出的),而有时我们可能会觉得这里的性能其实没那么重要
。这的确需要我们根据具体情况进行分析和权衡。emmy_debugger
中displayCustomTypeInfo
的设计。enableDisplayCustomTypeInfo
方法,而是在registerType中直接激活这一功能的建议,我认为这是一个简化用户操作的好方法。我会进行相应的修改。