Skip to content

Format files and add linter #201

Closed
abenso wants to merge 0 commit intomainfrom
chore/format
Closed

Format files and add linter #201
abenso wants to merge 0 commit intomainfrom
chore/format

Conversation

@abenso
Copy link
Copy Markdown
Collaborator

@abenso abenso commented Aug 4, 2025

🔗 zboto Link

Summary by CodeRabbit

  • Chores

    • Introduced a new automated workflow for code linting and formatting on key branches and pull requests.
    • Added a Makefile target to format source files using clang-format.
    • Updated patch version number.
  • Style

    • Applied consistent formatting and whitespace adjustments across multiple source and header files.
    • Enhanced UI macro definitions with additional interaction parameters for specific device targets.
  • Tests

    • Improved formatting and organization of test files for better readability.
    • Added new unit tests to expand coverage of formatting, conversion, and parsing functions.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Aug 4, 2025

Warning

Rate limit exceeded

@abenso has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 19 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between a4a0611 and 9833b90.

📒 Files selected for processing (1)
  • .github/workflows/lint.yml (1 hunks)

Walkthrough

This change set introduces a new GitHub Actions workflow for C/C++ linting and formatting, adds a format target to the Makefile, increments the patch version in the version header, and applies extensive code formatting and stylistic cleanups across source and test files. Several new unit tests are added to expand coverage of utility macros and string/integer conversion functions.

Changes

Cohort / File(s) Change Summary
CI Workflow
.github/workflows/lint.yml
Adds a GitHub Actions workflow for linting and formatting C/C++ code using clang-format and clang-tidy, with configuration for branch triggers and exclusions.
Build System
Makefile
Adds a format phony target to run clang-format on all source files in key directories.
Version Header
include/zxversion.h
Increments the ZXLIB_PATCH macro from 0 to 1.
EVM Source Formatting
evm/crypto_evm.c, evm/parser_impl_evm.c, evm/tx_evm.h
Applies whitespace and formatting adjustments; no logic changes.
Stack Overflow Handler
src/zxmacros.c
Changes infinite loop formatting in handle_stack_overflow; logic unchanged.
UI Source Formatting
app/ui/view.c, app/ui/view.h, app/ui/view_inspect_x.c, app/ui/view_nano.h, app/ui/view_nano_inspect.c, app/ui/view_nbgl.c, app/ui/view_s.c, app/ui/view_x.c
Applies formatting, indentation, and include order adjustments; minor signature formatting changes; no logic changes.
Header Comment and Macro Formatting
include/base64.h, include/bech32.h, include/bignum.h, include/view_templates.h, include/zxmacros.h
Reformats license headers and macros for consistent style; adds fields to UI_Icon macro for Nano targets; minor macro formatting improvements.
Test Formatting
tests/asciify.cpp, tests/base64.cpp, tests/bech32.cpp, tests/bip44path.cpp, tests/buffering_tests.cpp, tests/doubledabble.cpp, tests/hexutils.cpp, tests/sigutils.cpp, tests/zxformat.cpp
Reformats license headers, includes, indentation, and test case structure for consistency and readability; no test logic changes except minor assertion additions.
Expanded Macro Tests
tests/macros.cpp
Adds numerous new unit tests for string/integer conversion macros and functions, and reorganizes formatting for consistency.

Sequence Diagram(s)

sequenceDiagram
    participant GitHub
    participant Runner
    participant Linter

    GitHub->>Runner: Trigger workflow (push/PR/manual)
    Runner->>Runner: Checkout repo (recursive submodules)
    Runner->>Runner: Install dependencies (clang-format, clang-tidy, etc.)
    Runner->>Linter: Run cpp-linter-action
    Linter->>Runner: Annotate, summarize, comment on lint results
    Runner->>Runner: Fail workflow if lint/format errors detected
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/format

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Comment thread .github/workflows/lint.yml Fixed
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

♻️ Duplicate comments (3)
tests/bip44path.cpp (1)

16-18: Same missing-header problem as commented in tests/asciify.cpp – address
centrally in the workflow to avoid per-file suppressions.

tests/zxformat.cpp (1)

16-18: Duplicate of earlier comment: Google Mock headers are not available in the CI
image; install libgmock-dev or adjust the include.

.github/workflows/lint.yml (1)

11-14: Harden the workflow by scoping GITHUB_TOKEN permissions

The CodeQL warning is valid: jobs run with full write permissions by default.
Add an explicit, minimal permissions: block to the job (or workflow) level:

 jobs:
   lint:
+    permissions:
+      contents: read          # required by checkout
+      pull-requests: write    # needed for inline annotations/thread-comments
     runs-on: ubuntu-latest

This mitigates the risk of privilege escalation from compromised third-party
actions.

🧹 Nitpick comments (2)
tests/hexutils.cpp (1)

21-21: Include-order nit (optional)

If you keep the quoted form, group project headers after STL and before third-party (gmock) headers to follow common conventions.

.github/workflows/lint.yml (1)

18-41: Minor YAML hygiene – strip trailing whitespace & ensure final EOL

yaml-lint flagged three trailing-space lines and a missing newline at EOF.
They don’t break the workflow but will keep failing lint:

-          submodules: recursive␠
+          submodules: recursive-          
+-          
+

Also append a newline after Line 46.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 91be710 and 7a892ad.

📒 Files selected for processing (17)
  • .github/workflows/lint.yml (1 hunks)
  • Makefile (2 hunks)
  • evm/crypto_evm.c (1 hunks)
  • evm/parser_impl_evm.c (1 hunks)
  • evm/tx_evm.h (1 hunks)
  • include/zxversion.h (1 hunks)
  • src/zxmacros.c (1 hunks)
  • tests/asciify.cpp (1 hunks)
  • tests/base64.cpp (1 hunks)
  • tests/bech32.cpp (1 hunks)
  • tests/bip44path.cpp (1 hunks)
  • tests/buffering_tests.cpp (1 hunks)
  • tests/doubledabble.cpp (3 hunks)
  • tests/hexutils.cpp (1 hunks)
  • tests/macros.cpp (1 hunks)
  • tests/sigutils.cpp (4 hunks)
  • tests/zxformat.cpp (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
tests/hexutils.cpp

[failure] 17-17: tests/hexutils.cpp:17:10 [clang-diagnostic-error]
'hexutils.h' file not found

tests/bip44path.cpp

[failure] 16-16: tests/bip44path.cpp:16:10 [clang-diagnostic-error]
'gmock/gmock.h' file not found

tests/doubledabble.cpp

[failure] 17-17: tests/doubledabble.cpp:17:10 [clang-diagnostic-error]
'hexutils.h' file not found

tests/base64.cpp

[failure] 16-16: tests/base64.cpp:16:10 [clang-diagnostic-error]
'base64.h' file not found

tests/asciify.cpp

[failure] 16-16: tests/asciify.cpp:16:10 [clang-diagnostic-error]
'gmock/gmock.h' file not found

tests/bech32.cpp

[failure] 16-16: tests/bech32.cpp:16:10 [clang-diagnostic-error]
'bech32.h' file not found

tests/sigutils.cpp

[failure] 17-17: tests/sigutils.cpp:17:10 [clang-diagnostic-error]
'sigutils.h' file not found

tests/buffering_tests.cpp

[failure] 17-17: tests/buffering_tests.cpp:17:10 [clang-diagnostic-error]
'buffering.h' file not found

tests/zxformat.cpp

[failure] 16-16: tests/zxformat.cpp:16:10 [clang-diagnostic-error]
'gmock/gmock.h' file not found

tests/macros.cpp

[failure] 16-16: tests/macros.cpp:16:10 [clang-diagnostic-error]
'gmock/gmock.h' file not found

🪛 GitHub Check: CodeQL
.github/workflows/lint.yml

[warning] 13-46: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}

🪛 YAMLlint (1.37.1)
.github/workflows/lint.yml

[error] 18-18: trailing spaces

(trailing-spaces)


[error] 27-27: trailing spaces

(trailing-spaces)


[error] 41-41: trailing spaces

(trailing-spaces)


[error] 46-46: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (c-cpp)
🔇 Additional comments (17)
evm/tx_evm.h (1)

38-38: Formatting-only change is fine

Only an extra space was added after the #endif. No functional impact – looks good.

include/zxversion.h (1)

20-20: Version bump acknowledged

Patch version increment from 0 → 1 reflects the non-functional formatting & tooling updates. No issues.

evm/crypto_evm.c (1)

183-184: LGTM! Formatting improvement enhances readability.

The indentation alignment of the &evm_chain_code parameter improves code consistency and readability.

src/zxmacros.c (1)

31-32: LGTM! Improved formatting of infinite loop.

Expanding the infinite loop to multiple lines with an explicit semicolon improves readability and code style consistency.

evm/parser_impl_evm.c (1)

17-327: LGTM! Comprehensive formatting cleanup enhances codebase consistency.

The extensive formatting improvements throughout the file enhance readability and align with standard C coding conventions. All functional logic remains unchanged.

Makefile (2)

4-4: LGTM! Proper addition to phony targets.

Adding format to the .PHONY declaration ensures the target runs correctly even if a file named "format" exists.


31-32: LGTM! Useful formatting target for local development.

The new format target provides a convenient way for developers to apply consistent code formatting locally. The implementation correctly targets the appropriate directories and file extensions, and uses clang-format -i for in-place formatting.

tests/buffering_tests.cpp (1)

1-205: LGTM! Formatting improvements enhance test readability.

The comprehensive formatting cleanup improves code consistency and test structure while maintaining all functional logic.

tests/bech32.cpp (3)

2-15: LGTM: License header formatting is consistent.

The license header has been properly formatted with consistent leading asterisks and spacing.


21-148: LGTM: Test formatting is consistent and comprehensive.

The test cases have been properly unindented and reformatted for consistency. All test logic appears unchanged, maintaining coverage for:

  • Basic Bech32/Bech32M encoding functionality
  • Error handling for oversized inputs (out of bounds)
  • Buffer size validation (buffer too small)
  • Both encoding variants (BECH32 and BECH32M)

The explicit namespace closing comment improves readability.


16-19: bech32.h Header Availability Verified

The bech32.h file was found at include/bech32.h, so the <bech32.h> include in tests/bech32.cpp is valid and no missing dependencies were detected.

– No further action required.

tests/macros.cpp (6)

2-15: LGTM: License header formatting is consistent.

The license header has been properly formatted matching the project-wide style improvements.


21-30: LGTM: Hex string conversion test is correct.

The array_to_hexstr test properly validates hex conversion with appropriate buffer management and null termination checks.


32-55: LGTM: Fixed-point formatting tests are comprehensive.

The fpuint64_to_str tests cover various decimal place scenarios and validate the correct placement of decimal points. The test cases demonstrate proper understanding of fixed-point representation.


102-185: LGTM: Excellent boundary and error condition testing.

The error condition tests (fpstr_to_str_BAD_*) are particularly valuable as they validate:

  • Buffer overflow protection
  • Precision limit handling
  • Output buffer size constraints
  • Graceful "ERR" responses for invalid scenarios

This thorough error testing improves code reliability.


327-413: LGTM: Integer conversion tests cover edge cases well.

The UINT32_TO_STR and INT64_TO_STR test suites comprehensively validate:

  • Zero and boundary values (min/max)
  • Buffer size requirements
  • Positive and negative numbers
  • Proper error handling for insufficient buffer space

The use of std::numeric_limits for boundary testing is a good practice.


415-535: LGTM: String-to-integer parsing tests are thorough.

The STR_TO_INT8 and STR_TO_INT64 test suites provide excellent coverage for:

  • Boundary value validation (min/max ranges)
  • Invalid input handling (non-numeric characters)
  • Overflow/underflow detection
  • Proper error flag usage

The tests appropriately validate both successful parsing and error conditions.

Comment thread .github/workflows/lint.yml Outdated
Comment thread tests/asciify.cpp
Comment on lines 16 to +17
#include <gmock/gmock.h>
#include <zxmacros.h>
#include <zxformat.h>
#include <zxmacros.h>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Install Google Test/Mock headers to unblock clang-tidy include resolution

clang-tidy inside the new linter job errors out with
'gmock/gmock.h' file not found.
The test targets rely on Google Mock, but the workflow only installs clang-*
tooling, not the headers.
Either:

  1. Extend the apt-get install list with libgtest-dev libgmock-dev
    (preferred, keeps the test code unchanged), or
  2. Switch the include to <gtest/gtest.h> if you only need gtest macros and
    not gmock (verify first).

Without one of these, the linter workflow will always fail on every PR.

🧰 Tools
🪛 GitHub Check: lint

[failure] 16-16: tests/asciify.cpp:16:10 [clang-diagnostic-error]
'gmock/gmock.h' file not found

🤖 Prompt for AI Agents
In tests/asciify.cpp around lines 16 to 18, the include directive for
'gmock/gmock.h' causes clang-tidy to fail due to missing Google Mock headers. To
fix this, update the CI workflow to install the 'libgtest-dev' and
'libgmock-dev' packages via apt-get so the headers are available, or
alternatively, if only Google Test macros are needed, replace the include with
'gtest/gtest.h'. The preferred fix is to install the missing packages to keep
the test code unchanged.

Comment thread tests/base64.cpp
#include "base64.h"

#include <gmock/gmock.h>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Header cannot be found by the linter – supply include path or install dev package

base64.h lives in the repository but the linter compiles each file in
isolation and therefore does not know the include search-path.
Add the repo root (or src/ if that’s where the header sits) to
extra-arg: "-I$GITHUB_WORKSPACE" when calling the linter, or pass a
compile_commands.json. Otherwise every file that includes project headers
will keep triggering 'file not found' diagnostics.

🧰 Tools
🪛 GitHub Check: lint

[failure] 16-16: tests/base64.cpp:16:10 [clang-diagnostic-error]
'base64.h' file not found

🤖 Prompt for AI Agents
In tests/base64.cpp around lines 16 to 18, the linter cannot find the base64.h
header because it lacks the proper include path. To fix this, update the linter
configuration to add the repository root or the src/ directory to the include
search path by passing extra-arg: "-I$GITHUB_WORKSPACE" or provide a
compile_commands.json file so the linter knows where to find project headers and
stops reporting 'file not found' errors.

Comment thread tests/buffering_tests.cpp Outdated
********************************************************************************/

#include "gtest/gtest.h"
#include "buffering.h"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Fix missing header file compilation error.

The #include "buffering.h" is causing a compilation failure as indicated by the static analysis. This will prevent the tests from building successfully.

Verify the location and availability of the buffering.h header file:


🏁 Script executed:

#!/bin/bash
# Description: Find the buffering.h header file and check if it exists
# Expected: Should find the header file location or confirm it's missing

echo "Searching for buffering.h in the repository..."
fd -t f -e h "buffering"

echo -e "\nChecking if buffering.h exists in common include paths..."
fd -t f "buffering.h"

echo -e "\nLooking for buffering-related files..."
fd -t f "buffering"

Length of output: 495


Add test include path for buffering.h

The #include "buffering.h" in tests/buffering_tests.cpp is correct—buffering.h lives at include/buffering.h. The compilation failure simply means the compiler isn’t looking in include/. Please update your test build configuration (e.g., CMakeLists.txt) to add the include/ directory to the test target’s include paths. For example:

# In your CMakeLists.txt for the tests target
target_include_directories(buffering_tests PRIVATE
    ${CMAKE_SOURCE_DIR}/include
)

This ensures the compiler can locate buffering.h when building the tests.

🧰 Tools
🪛 GitHub Check: lint

[failure] 17-17: tests/buffering_tests.cpp:17:10 [clang-diagnostic-error]
'buffering.h' file not found

🤖 Prompt for AI Agents
In tests/buffering_tests.cpp at line 17, the include directive for "buffering.h"
fails because the compiler does not search the include/ directory by default. To
fix this, update the test build configuration file (such as CMakeLists.txt) to
add the include/ directory to the test target's include paths using
target_include_directories with the path ${CMAKE_SOURCE_DIR}/include. This will
allow the compiler to locate buffering.h during test compilation.

Comment thread tests/doubledabble.cpp Outdated
Comment on lines 17 to 20
#include <hexutils.h>

#include "bignum.h"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

hexutils.h still missing from include search path

Even with the angle-bracket form, hexutils.h cannot be located when building this test. Either:

  1. Add the appropriate include directory (e.g., -Iinclude) to the test build; or
  2. Use a relative path that resolves (#include "../include/hexutils.h").

Without this the new linter job – and any future build – will fail.

🧰 Tools
🪛 GitHub Check: lint

[failure] 17-17: tests/doubledabble.cpp:17:10 [clang-diagnostic-error]
'hexutils.h' file not found

🤖 Prompt for AI Agents
In tests/doubledabble.cpp around lines 17 to 19, the include directive for
hexutils.h uses angle brackets but the header file is not found in the include
search path during build. Fix this by either adding the directory containing
hexutils.h to the test build include paths (e.g., adding -Iinclude to the
compiler flags) or by changing the include directive to use a relative path that
correctly locates the file, such as #include "../include/hexutils.h".

Comment thread tests/hexutils.cpp Outdated
Comment on lines +17 to +21
#include "hexutils.h"

#include <string>

#include "hexutils.h"
#include "gmock/gmock.h"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

hexutils.h cannot be found – switch back to angle-bracket include or fix include-path

The linter error confirms the compiler can no longer locate the header. Using quotes makes the include relative to the current directory, whereas the project previously relied on the header being in the global include path.

-#include "hexutils.h"
+#include <hexutils.h>

Alternatively, add the directory that contains hexutils.h to the compiler’s include paths for the test target.

Failing to address this will break every CI build.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#include "hexutils.h"
#include <string>
#include "hexutils.h"
#include "gmock/gmock.h"
#include <hexutils.h>
#include <string>
#include "gmock/gmock.h"
🧰 Tools
🪛 GitHub Check: lint

[failure] 17-17: tests/hexutils.cpp:17:10 [clang-diagnostic-error]
'hexutils.h' file not found

🤖 Prompt for AI Agents
In tests/hexutils.cpp around lines 17 to 21, the include directive for
"hexutils.h" uses quotes which causes the compiler to look relative to the
current directory and fails to find the header. Change the include directive to
use angle brackets instead, i.e., #include <hexutils.h>, or alternatively,
update the test target's compiler include paths to include the directory
containing hexutils.h so the header can be found globally.

Comment thread tests/macros.cpp
Comment on lines 16 to +17
#include <gmock/gmock.h>
#include <zxmacros.h>
#include <zxformat.h>
#include <zxmacros.h>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify gmock availability.

Static analysis reports that gmock/gmock.h cannot be found. Please ensure Google Mock is properly installed and accessible in the include path.


🏁 Script executed:

#!/bin/bash
# Description: Check for gmock/gtest availability and include paths
echo "Checking for gmock headers..."
fd -t f "gmock.h" 2>/dev/null || echo "gmock.h not found"

echo -e "\nChecking for gtest/gmock in common locations..."
find /usr -name "*gmock*" -o -name "*gtest*" 2>/dev/null | head -10 || echo "Not found in /usr"

echo -e "\nChecking if this is a CMake project with find_package..."
fd -t f "CMakeLists.txt" --exec grep -l "GTest\|gmock" {}

Length of output: 461


🏁 Script executed:

#!/bin/bash
# Inspect how GTest/GMock are configured in CMakeLists.txt
echo "Searching for GTest/GMock configuration in CMakeLists.txt..."
grep -nE '(find_package|target_link_libraries|include_directories).*(GTest|gmock)' -C3 CMakeLists.txt

Length of output: 353


🏁 Script executed:

#!/bin/bash
# Locate where tests/macros.cpp is added and how GMock is linked
echo "Searching for macros.cpp references:"
grep -R -n "macros\.cpp" .

echo -e "\nInspecting CMakeLists.txt for test target linkage:"
grep -R -nE "add_executable|target_link_libraries" --include CMakeLists.txt .

Length of output: 457


🏁 Script executed:

#!/bin/bash
# Display CMake test target configuration to verify GMock linkage
sed -n '55,85p' CMakeLists.txt

Length of output: 511


Link and Configure Google Mock in CMake

GMock isn’t currently found or linked—even though tests/macros.cpp includes <gmock/gmock.h>, the CMake setup only finds and links GTest. You need to ensure GMock headers are on the include path and the test executable links against the GMock targets.

Locations to update:

  • In CMakeLists.txt around the existing find step (currently at line 40):
    • Change
      find_package(GTest CONFIG REQUIRED)
      to
      find_package(GTest CONFIG REQUIRED COMPONENTS gmock gmock_main)
  • In the target_link_libraries for zxlib_tests (around line 72):
    • Add GTest::gmock_main (and/or GTest::gmock) alongside GTest::gtest_main, for example:
      target_link_libraries(zxlib_tests PRIVATE
          GTest::gtest_main
  •   GTest::gmock_main
      zxlib)
    
    
    

After these changes, rerun CMake to verify <gmock/gmock.h> is found and the test target links correctly.

🧰 Tools
🪛 GitHub Check: lint

[failure] 16-16: tests/macros.cpp:16:10 [clang-diagnostic-error]
'gmock/gmock.h' file not found

🤖 Prompt for AI Agents
In CMakeLists.txt around lines 40 and 72, update the find_package call to
include GMock components by changing find_package(GTest CONFIG REQUIRED) to
find_package(GTest CONFIG REQUIRED COMPONENTS gmock gmock_main). Then, in the
target_link_libraries section for zxlib_tests near line 72, add
GTest::gmock_main alongside GTest::gtest_main to ensure GMock headers are
included and linked properly. After these changes, rerun CMake to confirm the
test executable finds and links GMock correctly.

Comment thread tests/sigutils.cpp Outdated
Comment on lines 17 to 19
#include "sigutils.h"

#include "gmock/gmock.h"
#include <string>
#include <hexutils.h>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

sigutils.h not resolved – header include style/path needs correction

sigutils.h now uses quotes, causing the same “file not found” error reported by clang-tidy. Use angle brackets or fix the include path:

-#include "sigutils.h"
+#include <sigutils.h>

Make sure the build system for the tests has the directory containing sigutils.h on the include search list.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#include "sigutils.h"
#include "gmock/gmock.h"
#include <string>
#include <hexutils.h>
#include <sigutils.h>
#include <hexutils.h>
🧰 Tools
🪛 GitHub Check: lint

[failure] 17-17: tests/sigutils.cpp:17:10 [clang-diagnostic-error]
'sigutils.h' file not found

🤖 Prompt for AI Agents
In tests/sigutils.cpp around lines 17 to 19, the include directive for
sigutils.h uses quotes but the file is not found, causing a build error. Change
the include to use angle brackets or adjust the include path so the compiler can
locate sigutils.h. Also verify that the test build configuration includes the
directory containing sigutils.h in its include search paths.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
.github/workflows/lint.yml (2)

11-14: Add explicit permissions block for GITHUB_TOKEN

The workflow uses the default token with full write permissions. Add an explicit, least-privilege permissions section (e.g., contents: read) at the job or workflow level.


19-26: Include Google Test/Mock dev packages so clang-tidy can parse tests

libgtest-dev and libgmock-dev are still missing from the apt-get install list, which breaks header resolution inside test files.

           sudo apt-get install -y \
             build-essential \
             cmake \
             clang-format \
             clang-tidy \
+            libgtest-dev \
+            libgmock-dev
🧹 Nitpick comments (4)
.github/workflows/lint.yml (4)

18-18: Remove trailing whitespace

YAMLlint flags trailing spaces on this line.


27-27: Remove trailing whitespace

Same lint warning as above for Line 27.


42-42: Remove trailing whitespace

Trailing spaces also appear on Line 42.


47-47: Add a newline at EOF

A final newline is required by POSIX tools and avoids YAMLlint new-line-at-end-of-file errors.

+
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 7a892ad and 1888298.

📒 Files selected for processing (1)
  • .github/workflows/lint.yml (1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/lint.yml

[warning] 13-47: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}

🪛 YAMLlint (1.37.1)
.github/workflows/lint.yml

[error] 18-18: trailing spaces

(trailing-spaces)


[error] 27-27: trailing spaces

(trailing-spaces)


[error] 42-42: trailing spaces

(trailing-spaces)


[error] 47-47: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: lint
  • GitHub Check: Analyze (c-cpp)
  • GitHub Check: lint

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 1888298 and 51b2589.

📒 Files selected for processing (15)
  • .github/workflows/lint.yml (1 hunks)
  • Makefile (2 hunks)
  • app/ui/view.c (1 hunks)
  • app/ui/view.h (2 hunks)
  • app/ui/view_inspect_x.c (5 hunks)
  • app/ui/view_nano.h (1 hunks)
  • app/ui/view_nano_inspect.c (4 hunks)
  • app/ui/view_nbgl.c (3 hunks)
  • app/ui/view_s.c (17 hunks)
  • app/ui/view_x.c (3 hunks)
  • include/base64.h (1 hunks)
  • include/bech32.h (1 hunks)
  • include/bignum.h (1 hunks)
  • include/view_templates.h (5 hunks)
  • include/zxmacros.h (1 hunks)
✅ Files skipped from review due to trivial changes (12)
  • include/zxmacros.h
  • app/ui/view_nano.h
  • Makefile
  • include/base64.h
  • app/ui/view_nbgl.c
  • app/ui/view_x.c
  • include/bignum.h
  • app/ui/view_inspect_x.c
  • include/bech32.h
  • app/ui/view_nano_inspect.c
  • app/ui/view_s.c
  • include/view_templates.h
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/lint.yml

[error] 21-21: trailing spaces

(trailing-spaces)


[error] 32-32: trailing spaces

(trailing-spaces)


[error] 47-47: trailing spaces

(trailing-spaces)


[error] 52-52: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (c-cpp)
🔇 Additional comments (3)
.github/workflows/lint.yml (1)

33-52: LGTM! Comprehensive linting configuration.

The cpp-linter configuration is well-structured with appropriate:

  • Include paths covering all source directories
  • Exclusions for build artifacts and configuration directories
  • Proper failure handling to enforce formatting standards
  • Integration with GitHub's annotation and commenting features

This addresses the previous concerns about missing dependencies and provides robust CI enforcement for code formatting.

app/ui/view.h (1)

123-124: LGTM! Consistent function declaration formatting.

The consolidation of the multi-line function declaration into a single line improves readability and consistency with the project's formatting standards. No functional changes were made.

app/ui/view.c (1)

65-66: LGTM! Consistent function definition formatting.

The consolidation of the multi-line function definition into a single line maintains consistency with the corresponding header file change and improves code formatting uniformity. The function logic and parameters remain unchanged.

Comment thread .github/workflows/lint.yml Outdated
- uses: actions/checkout@v4
with:
submodules: recursive

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix YAML formatting issues.

Static analysis detected several formatting issues that should be addressed:

-      
+
-          
+
-          
+

Also add a newline at the end of the file:

           echo "Linter or formatter failed!"
           exit 1
+

Also applies to: 32-32, 47-47, 52-52

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 21-21: trailing spaces

(trailing-spaces)

🤖 Prompt for AI Agents
In .github/workflows/lint.yml at lines 21, 32, 47, and 52, fix the YAML
formatting issues by correcting indentation, spacing, or syntax errors as
indicated by static analysis. Also, ensure there is a newline character at the
end of the file to comply with standard file formatting conventions.

@abenso abenso force-pushed the chore/format branch 2 times, most recently from 8c0594b to a4a0611 Compare August 4, 2025 19:01
@abenso abenso closed this Aug 4, 2025
@abenso abenso deleted the chore/format branch August 4, 2025 19:21
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