fix: fix override path matching in reload hotloading#143
Merged
Conversation
Reviewer's GuideReplaces File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这份代码变更主要包含两个方面的改进：将头文件中的静态函数改为内联函数，以及优化日志输出格式和循环变量引用。以下是对这些变更的详细审查和改进建议： 1. 头文件函数内联化 (helper.hpp)变更内容： 审查意见：
改进建议：
2. 日志格式化字符串修正 (dconfigrefmanager.cpp, dconfigresource.cpp, dconfigserver.cpp)变更内容： 审查意见：
改进建议：
3. 循环变量改为常量引用 (dconfigserver.cpp, mainwindow.cpp)变更内容： 审查意见：
改进建议：
总结这份代码变更整体质量很高，主要是针对 C++ 最佳实践 的修正：
最终评价：Approve。这些改进有助于提高程序的维护性、性能和健壮性。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- For the logging changes that switch
%dto%lld, the containersize()/count()methods typically returnintorqsizetyperather thanlong long, so consider casting tolong long(e.g.static_cast<long long>(keys.size())) or using a format specifier that matches the actual type to avoid UB and keep format/argument alignment strict. - In
loadTranslation, the range-based loop still usesfor (auto item : translationDirs()), which copies eachQString; consider making this consistent with the other changes by usingfor (const auto &item : translationDirs())to avoid unnecessary copies. - Changing
staticfunctions in a header to plaininlinealters their linkage; if these helpers are intended to remain TU-local or to avoid any risk of ODR issues, consider whetherstatic inlineor keeping them in an anonymous namespace would better match the original intent while still resolving the linker warnings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the logging changes that switch `%d` to `%lld`, the container `size()`/`count()` methods typically return `int` or `qsizetype` rather than `long long`, so consider casting to `long long` (e.g. `static_cast<long long>(keys.size())`) or using a format specifier that matches the actual type to avoid UB and keep format/argument alignment strict.
- In `loadTranslation`, the range-based loop still uses `for (auto item : translationDirs())`, which copies each `QString`; consider making this consistent with the other changes by using `for (const auto &item : translationDirs())` to avoid unnecessary copies.
- Changing `static` functions in a header to plain `inline` alters their linkage; if these helpers are intended to remain TU-local or to avoid any risk of ODR issues, consider whether `static inline` or keeping them in an anonymous namespace would better match the original intent while still resolving the linker warnings.
## Individual Comments
### Comment 1
<location path="dconfig-center/dde-dconfig-daemon/dconfigserver.cpp" line_range="360" />
<code_context>
{
const QList<ConfigCacheKey> &keys = request.data;
- qCInfo(cfLog, "Do sync config cache, keys count:%d", keys.size());
+ qCInfo(cfLog, "Do sync config cache, keys count:%lld", keys.size());
for (auto key: keys) {
auto resourceKey = getResourceKeyByConfigCache(key);
</code_context>
<issue_to_address>
**issue (bug_risk):** The `%lld` format specifier likely mismatches the type of `keys.size()`.
`QList::size()` / `count()` returns `int` (Qt5) or `qsizetype` (Qt6), neither of which is guaranteed to be `long long`, so `%lld` is technically UB even if it seems to work. Please either:
- keep `%d` and cast to `int` if the size is known to fit, or
- cast to `long long` and keep `%lld`, or
- use `%zd` with a cast to `size_t` / `qsizetype`.
The format specifier should match the actual argument type.
</issue_to_address>
### Comment 2
<location path="dconfig-center/dde-dconfig-daemon/dconfigresource.cpp" line_range="443" />
<code_context>
}
- qDebug(cfLog, "Removed connection:%s, remaining %d connection.", qPrintable(connKey), m_conns.count());
+ qDebug(cfLog, "Removed connection:%s, remaining %lld connection.", qPrintable(connKey), m_conns.count());
}
</code_context>
<issue_to_address>
**issue (bug_risk):** `%lld` may not match the type returned by `m_conns.count()`.
`m_conns.count()` is `int` (Qt5) or `qsizetype` (Qt6), which may not be `long long`, so using `%lld` without a cast is technically UB. Please either use `%d` to match `int` or explicitly cast to `long long`, e.g. `static_cast<long long>(m_conns.count())`, when using `%lld`.
</issue_to_address>
### Comment 3
<location path="dconfig-center/dde-dconfig-daemon/dconfigresource.cpp" line_range="453" />
<code_context>
void DSGConfigResource::save()
{
- qDebug(cfLog, "Save resource's cache for [%s], and cache count:%d", qPrintable(m_key), m_caches.count());
+ qDebug(cfLog, "Save resource's cache for [%s], and cache count:%lld", qPrintable(m_key), m_caches.count());
for (auto item : m_files)
item->save(m_localPrefix);
</code_context>
<issue_to_address>
**issue (bug_risk):** Format specifier for `m_caches.count()` should be aligned with its actual type.
`m_caches.count()` is unlikely to be `long long`, so `%lld` may not match its actual type. Either use `%d` if `count()` is `int`, or keep `%lld` and cast explicitly: `static_cast<long long>(m_caches.count())`. Apply the same approach consistently in all similar logging calls to avoid UB from format/type mismatches.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| { | ||
| const QList<ConfigCacheKey> &keys = request.data; | ||
| qCInfo(cfLog, "Do sync config cache, keys count:%d", keys.size()); | ||
| qCInfo(cfLog, "Do sync config cache, keys count:%lld", keys.size()); |
There was a problem hiding this comment.
issue (bug_risk): The %lld format specifier likely mismatches the type of keys.size().
QList::size() / count() returns int (Qt5) or qsizetype (Qt6), neither of which is guaranteed to be long long, so %lld is technically UB even if it seems to work. Please either:
- keep
%dand cast tointif the size is known to fit, or - cast to
long longand keep%lld, or - use
%zdwith a cast tosize_t/qsizetype.
The format specifier should match the actual argument type.
| } | ||
|
|
||
| qDebug(cfLog, "Removed connection:%s, remaining %d connection.", qPrintable(connKey), m_conns.count()); | ||
| qDebug(cfLog, "Removed connection:%s, remaining %lld connection.", qPrintable(connKey), m_conns.count()); |
There was a problem hiding this comment.
issue (bug_risk): %lld may not match the type returned by m_conns.count().
m_conns.count() is int (Qt5) or qsizetype (Qt6), which may not be long long, so using %lld without a cast is technically UB. Please either use %d to match int or explicitly cast to long long, e.g. static_cast<long long>(m_conns.count()), when using %lld.
| void DSGConfigResource::save() | ||
| { | ||
| qDebug(cfLog, "Save resource's cache for [%s], and cache count:%d", qPrintable(m_key), m_caches.count()); | ||
| qDebug(cfLog, "Save resource's cache for [%s], and cache count:%lld", qPrintable(m_key), m_caches.count()); |
There was a problem hiding this comment.
issue (bug_risk): Format specifier for m_caches.count() should be aligned with its actual type.
m_caches.count() is unlikely to be long long, so %lld may not match its actual type. Either use %d if count() is int, or keep %lld and cast explicitly: static_cast<long long>(m_caches.count()). Apply the same approach consistently in all similar logging calls to avoid UB from format/type mismatches.
Contributor
|
TAG Bot New tag: 1.0.42 |
The issue was that during hot reload operations, override configuration paths were being incorrectly matched by the getMetaConfigureId function instead of being handled by getOverrideConfigureId. This caused override configurations to fail during reload operations. The fix modifies the regular expression in getMetaConfigureId to exclude override paths using negative lookahead (?!overrides/). This ensures that override paths are properly routed to getOverrideConfigureId for correct processing. Additionally, the getConfigureIdByPath function was updated to not check for file existence before parsing, as files may not exist during deletion operations but still need path parsing. Log: Fixed hot reload functionality for override configuration paths Influence: 1. Test hot reload functionality with override configurations 2. Verify that override paths are correctly identified and processed 3. Test configuration deletion and reload scenarios 4. Validate both standard and override configuration paths work correctly 5. Test edge cases with special characters in configuration paths fix: 修复热加载中覆盖路径匹配问题 问题在于热重载操作期间，覆盖配置路径被 getMetaConfigureId 函数错误匹配， 而不是由 getOverrideConfigureId 处理。这导致覆盖配置在重载操作期间失效。 修复方案修改了 getMetaConfigureId 中的正则表达式，使用负向先 行断言 (?!overrides/) 排除覆盖路径。这确保覆盖路径能正确路由到 getOverrideConfigureId 进行处理。此外，更新了 getConfigureIdByPath 函 数，使其在解析前不检查文件是否存在，因为在删除操作期间文件可能不存在但仍 需要路径解析。 Log: 修复了覆盖配置路径的热重载功能 Influence: 1. 测试覆盖配置的热重载功能 2. 验证覆盖路径是否正确识别和处理 3. 测试配置删除和重载场景 4. 验证标准和覆盖配置路径都能正常工作 5. 测试包含特殊字符的配置路径边界情况
18202781743
changed the title
BLumia
approved these changes
Mar 24, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, BLumia The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The issue was that during hot reload operations, override configuration
paths were being incorrectly matched by the getMetaConfigureId function
instead of being handled by getOverrideConfigureId. This caused override
configurations to fail during reload operations.
The fix modifies the regular expression in getMetaConfigureId to exclude
override paths using negative lookahead (?!overrides/). This ensures
that override paths are properly routed to getOverrideConfigureId for
correct processing. Additionally, the getConfigureIdByPath function was
updated to not check for file existence before parsing, as files may not
exist during deletion operations but still need path parsing.
Log: Fixed hot reload functionality for override configuration paths
Influence:
correctly
fix: 修复热加载中覆盖路径匹配问题
问题在于热重载操作期间，覆盖配置路径被 getMetaConfigureId 函数错误匹配，
而不是由 getOverrideConfigureId 处理。这导致覆盖配置在重载操作期间失效。
修复方案修改了 getMetaConfigureId 中的正则表达式，使用负向先
行断言 (?!overrides/) 排除覆盖路径。这确保覆盖路径能正确路由到
getOverrideConfigureId 进行处理。此外，更新了 getConfigureIdByPath 函
数，使其在解析前不检查文件是否存在，因为在删除操作期间文件可能不存在但仍
需要路径解析。
Log: 修复了覆盖配置路径的热重载功能
Influence: