Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes clazy-fully-qualified-moc-types: MOC requires enum types in slot/signal declarations to be fully qualified. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prevents implicit detachment of Qt containers when iterating with range-based for loops. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace last() with constLast() and operator[]() with at() on temporary QList values to avoid detachment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Loop counters used types narrower than qsizetype (the return type of QList::size()), causing potential truncation on large lists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merged consecutive branches with identical bodies into single conditions: - communicationstats: combine size()==0 and size()<=1 checks - mbcregisterfilter: simplify tab filter to single boolean expression - settingsauto: merge two group-separator space-replacement branches - datafileexporter: merge skip-line conditions for notes and empty comments - graphview: invert handled-move check to remove two empty branches - fileselectionhelper: merge FILE_TYPE_NONE and default break cases Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reorder fields in _ConnectionSettings struct to minimize padding. Group large/aligned types (QString, quint32, quint16) before bool flags, reducing padding from 42 bytes to 2 bytes optimal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughAdds static-analysis CI jobs and local automation scripts, a clang-tidy configuration, enables compile command export, and applies widespread code formatting/type-safety/iteration refinements across multiple modules and headers. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/importexport/datafileexporter.cpp (1)
421-426:⚠️ Potential issue | 🟡 MinorMissing
breakstatement causes fall-through todefault.The
case E_VALUE_AXIS:is missing abreak;statement afterline.append("Axis");, causing implicit fall-through todefault:. While currently benign (default only breaks), this is a latent defect that will cause bugs if code is added todefault:, and may trigger-Wimplicit-fallthroughwarnings.🐛 Proposed fix
case E_VALUE_AXIS: line.append("Axis"); + break; default: break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/datafileexporter.cpp` around lines 421 - 426, Add a missing break in the switch handling for E_VALUE_AXIS: after the line.append("Axis"); statement in the switch (case E_VALUE_AXIS) add a break to prevent fall-through into default and avoid future bugs or -Wimplicit-fallthrough warnings; update the switch that contains E_VALUE_AXIS / line.append("Axis") so case E_VALUE_AXIS ends with a break before default.
🧹 Nitpick comments (6)
src/importexport/datafileexporter.cpp (3)
521-525: Consider adding error handling for failed file operations.The
(void)cast suppresses the warning, but ifopen()fails, the file won't be cleared and no error is reported. If this is intentional best-effort behaviour, a brief comment would clarify intent. Otherwise, consider logging or propagating the failure.♻️ Optional: Add minimal error handling
void DataFileExporter::clearFile(QString filePath) { QFile file(filePath); - (void) file.open(QIODevice::WriteOnly | QIODevice::Text); // Remove all data from file + if (!file.open(QIODevice::WriteOnly | QIODevice::Text)) + { + // Best-effort clear; failure is non-fatal + return; + } + file.resize(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/datafileexporter.cpp` around lines 521 - 525, DataFileExporter::clearFile currently ignores QFile::open() failures; change it to check the boolean result of file.open(QIODevice::WriteOnly | QIODevice::Truncate | QIODevice::Text) and handle errors: if open() fails, log an error including file.errorString() (or propagate the error/return a status) so callers know the clear failed; alternatively, if silent best-effort behavior is intended, replace the casted call with a brief comment explaining that failure is ignored intentionally. Ensure references to DataFileExporter::clearFile and QFile::open/QFile::errorString are used to locate the change.
500-503: Replace deprecatedforeachmacro with range-based for loop.The
foreachmacro is deprecated in Qt and implicitly copies the container. Use a range-based for loop withstd::as_constfor consistency with line 192 and better performance.♻️ Proposed fix
- foreach (QString line, logData) + for (const QString& line : std::as_const(logData)) { stream << line << "\n"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/datafileexporter.cpp` around lines 500 - 503, Replace the deprecated Qt foreach usage that iterates over logData with a C++ range-based for that iterates over std::as_const(logData) and takes each element by const reference to avoid copying; specifically update the loop that currently uses foreach (QString line, logData) to use a range-for over std::as_const(logData) and write each line to stream with the same newline handling (preserve the body that does stream << line << "\n"); this mirrors the pattern used around line 192 and improves performance.
168-171: Consider inverting the condition to avoid empty if-body.The empty if-body with a comment is functional but unconventional. Inverting the condition would make the intent clearer.
♻️ Suggested refactor
- if (line.left(6).toLower() == "//note" || (bPreviousLineWasEmptyComment && bEmptyCommentLine)) - { - // Skip note lines and avoid 2 subsequent empty comment lines - } - else + bool bIsNoteLine = line.left(6).toLower() == "//note"; + bool bSkipDuplicateEmpty = bPreviousLineWasEmptyComment && bEmptyCommentLine; + + if (!bIsNoteLine && !bSkipDuplicateEmpty) { // copy line tmpStream << line << "\n"; bPreviousLineWasEmptyComment = bEmptyCommentLine; } + // else: Skip note lines and avoid 2 subsequent empty comment lines🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/datafileexporter.cpp` around lines 168 - 171, The empty-if that skips note/duplicate-empty-comment lines should be made explicit by inverting it into an early continue: replace the empty if (line.left(6).toLower() == "//note" || (bPreviousLineWasEmptyComment && bEmptyCommentLine)) { /* skip */ } with a check that calls continue when those conditions are true (e.g. if (line.left(6).toLower() == "//note" || (bPreviousLineWasEmptyComment && bEmptyCommentLine)) { continue; }) so subsequent code in the loop clearly handles only non-skipped lines; refer to the variables and expressions line.left(6).toLower(), bPreviousLineWasEmptyComment and bEmptyCommentLine to locate the change in the loop in datafileexporter.cpp.src/communication/registervaluehandler.cpp (1)
33-33: Inconsistent loop index type withstartRead().Line 18 correctly uses
qsizetypefor the loop index, but this loop usesqint32. For consistency and to avoid potential truncation on 64-bit systems with large lists, consider usingqsizetypehere as well.♻️ Proposed fix
- for (qint32 listIdx = 0; listIdx < _registerList.size(); listIdx++) + for (qsizetype listIdx = 0; listIdx < _registerList.size(); listIdx++)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/communication/registervaluehandler.cpp` at line 33, Replace the qint32 loop index with qsizetype to match startRead() and avoid truncation: in the for loop iterating _registerList (the loop using "for (qint32 listIdx = 0; listIdx < _registerList.size(); listIdx++)"), change the loop variable type to qsizetype (keeping the name listIdx) and keep the same condition and body; ensure the change is applied in registervaluehandler.cpp where _registerList is iterated so the index type is consistent with other uses like startRead().scripts/run_clang_tidy.sh (1)
17-17: Reconsider suppressing stderr entirely.Redirecting stderr to
/dev/nullmay hide legitimate error messages fromrun-clang-tidy, such as compilation errors or configuration issues. The-quietflag already reduces verbosity. Consider removing2>/dev/nullor redirecting to a log file for debugging purposes.♻️ Proposed fix
-run-clang-tidy -quiet -p "${BUILD_DIR}" -j "$(nproc)" "$(pwd)/src/.*\.cpp\$" 2>/dev/null +run-clang-tidy -quiet -p "${BUILD_DIR}" -j "$(nproc)" "$(pwd)/src/.*\.cpp\$"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run_clang_tidy.sh` at line 17, The script currently silences all stderr from the run-clang-tidy invocation (the command string starting with run-clang-tidy -quiet -p "${BUILD_DIR}" -j "$(nproc)" "$(pwd)/src/.*\.cpp\$"), which can hide real errors; update the invocation to stop redirecting stderr to /dev/null—either remove the trailing `2>/dev/null` or redirect stderr to a logfile (e.g., `2>>clang-tidy-errors.log`) so that compile/configuration errors from run-clang-tidy are preserved for debugging while keeping the -quiet flag.scripts/run_clang_format.sh (1)
9-14: Consider null-delimited output for filenames with spaces.The current pipeline uses newline-separated filenames, which could fail if any C++ file paths contain spaces. While uncommon, this can be made more robust.
♻️ Proposed fix using null-delimited output
CHANGED_FILES=$( - { git diff --name-only --diff-filter=d HEAD; git ls-files --others --exclude-standard; } \ - | grep -E '\.(cpp|h)$' \ - | sort -u \ + { git diff --name-only -z --diff-filter=d HEAD; git ls-files --others --exclude-standard -z; } \ + | tr '\0' '\n' \ + | grep -E '\.(cpp|h)$' \ + | sort -u \ || true )Alternatively, refactor to use
xargs -0throughout if null-delimited handling is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run_clang_format.sh` around lines 9 - 14, The CHANGED_FILES pipeline can break on filenames with spaces; switch to null-delimited output by adding -z to the git commands (use git diff --name-only --diff-filter=d -z and git ls-files --others --exclude-standard -z), use grep -z -E '\.(cpp|h)$' and sort -z -u (or consume the null stream into an array via read -d ''/mapfile -d '') so the rest of the script can safely handle filenames with spaces; alternatively ensure any downstream use of CHANGED_FILES uses xargs -0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 27-39: The fenced project-structure code block in CLAUDE.md is
missing a language tag causing markdownlint MD040; update the opening fence to
include a language identifier (e.g., use ```text) so the block becomes ```text
... ``` to satisfy the linter and preserve formatting.
In `@scripts/run_clazy.sh`:
- Around line 13-15: The script scripts/run_clazy.sh is missing the autogen
header generation step, causing Clazy to see missing/generated Qt headers;
before invoking clazy-standalone (the clazy run loop), run the same autogen
build step used in run_clang_tidy.sh — invoke ninja -C "${BUILD_DIR}"
src/ScopeSource_autogen (or the equivalent autogen target for your project) to
ensure MOC/UI headers are generated so clazy-standalone can analyze files that
depend on those headers.
In `@scripts/run_precommit.sh`:
- Line 5: The cd invocation using the SCRIPT_DIR variable can fail silently;
update the command that changes directory (the cd "${SCRIPT_DIR}/..") to handle
failure by appending a failure handler such as || exit 1 so the script aborts
immediately if the target directory doesn't exist or cd fails; ensure SCRIPT_DIR
is used as before and the change is only to the cd invocation.
In `@src/graphview/graphview.cpp`:
- Around line 366-369: The loop is modifying a temporary QPen returned by
_pPlot->graph(i)->pen(), so the width change is discarded; instead, for each
graph obtained via _pPlot->graph(i) copy its pen into a local QPen variable,
call setWidth(1) on that local pen, then restore it with
graph->setPen(modifiedPen) (use _pPlot->graph(i) to get the graph, QPen local
variable, setWidth, and graph->setPen to apply).
---
Outside diff comments:
In `@src/importexport/datafileexporter.cpp`:
- Around line 421-426: Add a missing break in the switch handling for
E_VALUE_AXIS: after the line.append("Axis"); statement in the switch (case
E_VALUE_AXIS) add a break to prevent fall-through into default and avoid future
bugs or -Wimplicit-fallthrough warnings; update the switch that contains
E_VALUE_AXIS / line.append("Axis") so case E_VALUE_AXIS ends with a break before
default.
---
Nitpick comments:
In `@scripts/run_clang_format.sh`:
- Around line 9-14: The CHANGED_FILES pipeline can break on filenames with
spaces; switch to null-delimited output by adding -z to the git commands (use
git diff --name-only --diff-filter=d -z and git ls-files --others
--exclude-standard -z), use grep -z -E '\.(cpp|h)$' and sort -z -u (or consume
the null stream into an array via read -d ''/mapfile -d '') so the rest of the
script can safely handle filenames with spaces; alternatively ensure any
downstream use of CHANGED_FILES uses xargs -0.
In `@scripts/run_clang_tidy.sh`:
- Line 17: The script currently silences all stderr from the run-clang-tidy
invocation (the command string starting with run-clang-tidy -quiet -p
"${BUILD_DIR}" -j "$(nproc)" "$(pwd)/src/.*\.cpp\$"), which can hide real
errors; update the invocation to stop redirecting stderr to /dev/null—either
remove the trailing `2>/dev/null` or redirect stderr to a logfile (e.g.,
`2>>clang-tidy-errors.log`) so that compile/configuration errors from
run-clang-tidy are preserved for debugging while keeping the -quiet flag.
In `@src/communication/registervaluehandler.cpp`:
- Line 33: Replace the qint32 loop index with qsizetype to match startRead() and
avoid truncation: in the for loop iterating _registerList (the loop using "for
(qint32 listIdx = 0; listIdx < _registerList.size(); listIdx++)"), change the
loop variable type to qsizetype (keeping the name listIdx) and keep the same
condition and body; ensure the change is applied in registervaluehandler.cpp
where _registerList is iterated so the index type is consistent with other uses
like startRead().
In `@src/importexport/datafileexporter.cpp`:
- Around line 521-525: DataFileExporter::clearFile currently ignores
QFile::open() failures; change it to check the boolean result of
file.open(QIODevice::WriteOnly | QIODevice::Truncate | QIODevice::Text) and
handle errors: if open() fails, log an error including file.errorString() (or
propagate the error/return a status) so callers know the clear failed;
alternatively, if silent best-effort behavior is intended, replace the casted
call with a brief comment explaining that failure is ignored intentionally.
Ensure references to DataFileExporter::clearFile and
QFile::open/QFile::errorString are used to locate the change.
- Around line 500-503: Replace the deprecated Qt foreach usage that iterates
over logData with a C++ range-based for that iterates over
std::as_const(logData) and takes each element by const reference to avoid
copying; specifically update the loop that currently uses foreach (QString line,
logData) to use a range-for over std::as_const(logData) and write each line to
stream with the same newline handling (preserve the body that does stream <<
line << "\n"); this mirrors the pattern used around line 192 and improves
performance.
- Around line 168-171: The empty-if that skips note/duplicate-empty-comment
lines should be made explicit by inverting it into an early continue: replace
the empty if (line.left(6).toLower() == "//note" ||
(bPreviousLineWasEmptyComment && bEmptyCommentLine)) { /* skip */ } with a check
that calls continue when those conditions are true (e.g. if
(line.left(6).toLower() == "//note" || (bPreviousLineWasEmptyComment &&
bEmptyCommentLine)) { continue; }) so subsequent code in the loop clearly
handles only non-skipped lines; refer to the variables and expressions
line.left(6).toLower(), bPreviousLineWasEmptyComment and bEmptyCommentLine to
locate the change in the loop in datafileexporter.cpp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1754e440-8176-4ddf-80fe-846bf3e25a6c
📒 Files selected for processing (29)
.clang-tidy.github/workflows/code_quality.ymlCLAUDE.mdCMakeLists.txtscripts/run_clang_format.shscripts/run_clang_tidy.shscripts/run_clazy.shscripts/run_precommit.shsrc/communication/communicationstats.cppsrc/communication/modbusconnection.cppsrc/communication/modbuspoll.cppsrc/communication/modbusregister.cppsrc/communication/readregisters.cppsrc/communication/registervaluehandler.cppsrc/customwidgets/deviceform.cppsrc/dialogs/devicesettings.cppsrc/dialogs/importmbcdialog.cppsrc/dialogs/mainwindow.cppsrc/dialogs/settingsdialog.cppsrc/graphview/graphscaling.cppsrc/graphview/graphview.cppsrc/importexport/datafileexporter.cppsrc/importexport/projectfiledata.hsrc/importexport/projectfileexporter.cppsrc/importexport/settingsauto.cppsrc/models/guimodel.hsrc/models/mbcregisterfilter.cppsrc/util/fileselectionhelper.cppsrc/util/modbusdatatype.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/run_clazy.sh`:
- Around line 8-11: The script uses the caller's working directory for paths
(cmake -B "${BUILD_DIR}" and scanning $(pwd)/src), which breaks when run outside
the repo root; compute a repository root (e.g., REPO_ROOT via git rev-parse
--show-toplevel or dirname "$0") and anchor all paths to it: pass the repo root
(or repo_root/src) as cmake's source dir and prefix BUILD_DIR and any scans
(replace $(pwd)/src) with "${REPO_ROOT}/${BUILD_DIR}" and "${REPO_ROOT}/src"
respectively, keeping QT_PREFIX usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12ff0c96-d452-4638-8994-c0d1a8c09fec
📒 Files selected for processing (4)
.github/workflows/code_quality.ymlscripts/run_clazy.shsrc/graphview/graphview.cppsrc/importexport/datafileexporter.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/importexport/datafileexporter.cpp
- .github/workflows/code_quality.yml
Comment on lines
+8
to
+11
| cmake -GNinja \ | ||
| -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ | ||
| -DCMAKE_PREFIX_PATH="${QT_PREFIX}" \ | ||
| -B "${BUILD_DIR}" |
There was a problem hiding this comment.
Anchor all paths to the repository root instead of the caller’s working directory.
Line 8 configures CMake without an explicit source dir, and Line 17 scans $(pwd)/src. This breaks when the script is launched outside the repo root.
🔧 Proposed fix
#!/bin/bash
set -euo pipefail
-BUILD_DIR="${1:-build}"
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+BUILD_DIR="${1:-build}"
QT_PREFIX="${2:-/opt/Qt/6.8.3/gcc_64}"
+
+if [[ "${BUILD_DIR}" != /* ]]; then
+ BUILD_DIR="${REPO_ROOT}/${BUILD_DIR}"
+fi
echo "=== Configuring (compile_commands.json) ==="
cmake -GNinja \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DCMAKE_PREFIX_PATH="${QT_PREFIX}" \
+ -S "${REPO_ROOT}" \
-B "${BUILD_DIR}"
@@
echo "=== Running clazy ==="
-find "$(pwd)/src" -name "*.cpp" -print0 | \
+find "${REPO_ROOT}/src" -name "*.cpp" -print0 | \
xargs -0 -P "$(nproc)" -I{} clazy-standalone --only-qt --header-filter="src/.*" -p "${BUILD_DIR}" {}Also applies to: 17-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/run_clazy.sh` around lines 8 - 11, The script uses the caller's
working directory for paths (cmake -B "${BUILD_DIR}" and scanning $(pwd)/src),
which breaks when run outside the repo root; compute a repository root (e.g.,
REPO_ROOT via git rev-parse --show-toplevel or dirname "$0") and anchor all
paths to it: pass the repo root (or repo_root/src) as cmake's source dir and
prefix BUILD_DIR and any scans (replace $(pwd)/src) with
"${REPO_ROOT}/${BUILD_DIR}" and "${REPO_ROOT}/src" respectively, keeping
QT_PREFIX usage unchanged.
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
1 participant
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.
Summary by CodeRabbit
Chores
Documentation
Refactor