fix(misc-redundant-expression): fix clang-tidy violations#2019
Open
github-actions[bot] wants to merge 3 commits intomainfrom
Open
fix(misc-redundant-expression): fix clang-tidy violations#2019github-actions[bot] wants to merge 3 commits intomainfrom
github-actions[bot] wants to merge 3 commits intomainfrom
Conversation
Collaborator
|
@broskoTT looks like clang-tidy + copilot caught a bug here. Could you please confirm? Are we missing a test? |
pjanevskiTT
approved these changes
Feb 2, 2026
Contributor
|
Ok won't merge until @broskoTT takes a look at the potential bug |
broskoTT
reviewed
Feb 2, 2026
Contributor
|
@blozano-tt I've opened a new pull request, #2049, to work on those changes. Once the pull request is ready, I'll request review from you. |
blozano-tt
added a commit
that referenced
this pull request
Feb 3, 2026
### Issue Addresses feedback on #2019: #2019 (comment) ### Description The original `spi_ctrl0_spi_scph()` function contained `(scph << 6) & 0x1`, which always returned 0 due to shift-then-mask order. Since this function was only used in bitwise OR expressions where OR-ing with 0 has no effect, and the SPI implementation works correctly without it, the function is dead code. Removed the function and all call sites. ### List of the changes - Removed `spi_ctrl0_spi_scph(uint32_t scph)` function definition - Removed function calls from three SPI control register writes in `init()`, `read_status()`, and `lock()` methods ### Testing Build verification confirms clean compilation. ### API Changes There are no API changes in this PR. <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: blozano-tt <181790211+blozano-tt@users.noreply.github.com>
…on (#2020) ### Issue Fix clang-tidy violations for checker: misc-redundant-expression ### Description Fixed ineffective bitwise operation in SPI control register helper. Original expression `(scph << 6) & 0x1` always evaluated to 0 due to shift-then-mask order. Corrected to `(scph & 0x1) << 6` to properly place bit 0 at position 6. ```cpp // Before: Always returns 0 static inline uint32_t spi_ctrl0_spi_scph(uint32_t scph) { return (scph << 6) & 0x1; } // After: Returns 0x00 or 0x40 based on input LSB static inline uint32_t spi_ctrl0_spi_scph(uint32_t scph) { return (scph & 0x1) << 6; } ``` ### List of the changes - Fixed redundant bitwise AND in `spi_ctrl0_spi_scph()` helper function - Removed `-misc-redundant-expression` from `.clang-tidy` to enable regression detection - Deleted `violations_to_fix.txt` ### Testing Build verification confirms clean compilation with fix applied. ### API Changes There are no API changes in this PR. <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > Fix all clang-tidy violations for checker: misc-redundant-expression > > The full list of violations is in: violations_to_fix.txt > > Some violations may have been auto-fixed by CodeChecker. > Review those fixes and fix any remaining violations. > > Total violations: 1 > > For each violation in violations_to_fix.txt: > 1. If already fixed by auto-fix, verify it's correct > 2. If not fixed, apply the appropriate fix > 3. Follow the existing code style > > After fixing all violations: > 1. Delete violations_to_fix.txt > 2. If .clang-tidy disables this checker (e.g., -misc-redundant-expression), > remove that disablement to enable the check for regressions </details> <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/tenstorrent/tt-umd/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: blozano-tt <181790211+blozano-tt@users.noreply.github.com>
### Issue Addresses feedback on #2019: #2019 (comment) ### Description The original `spi_ctrl0_spi_scph()` function contained `(scph << 6) & 0x1`, which always returned 0 due to shift-then-mask order. Since this function was only used in bitwise OR expressions where OR-ing with 0 has no effect, and the SPI implementation works correctly without it, the function is dead code. Removed the function and all call sites. ### List of the changes - Removed `spi_ctrl0_spi_scph(uint32_t scph)` function definition - Removed function calls from three SPI control register writes in `init()`, `read_status()`, and `lock()` methods ### Testing Build verification confirms clean compilation. ### API Changes There are no API changes in this PR. <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: blozano-tt <181790211+blozano-tt@users.noreply.github.com>
4e03e79 to
66728f0
Compare
blozano-tt
reviewed
Feb 10, 2026
|
|
||
| static constexpr uint32_t SPI_DUMP_ADDR_CORRECTION = 0x10000000; | ||
|
|
||
| static inline uint32_t spi_ctrl0_spi_scph(uint32_t scph) { return (scph << 6) & 0x1; } |
Collaborator
There was a problem hiding this comment.
Redundant expression always returns 0.
blozano-tt
reviewed
Feb 10, 2026
|
|
||
| val = SPI_CTRL0_TMOD_EEPROM_READ | SPI_CTRL0_SPI_FRF_STANDARD | SPI_CTRL0_DFS32_FRAME_08BITS | | ||
| spi_ctrl0_spi_scph(0x1); | ||
| val = SPI_CTRL0_TMOD_EEPROM_READ | SPI_CTRL0_SPI_FRF_STANDARD | SPI_CTRL0_DFS32_FRAME_08BITS; |
blozano-tt
reviewed
Feb 10, 2026
|
|
||
| val = SPI_CTRL0_TMOD_EEPROM_READ | SPI_CTRL0_SPI_FRF_STANDARD | SPI_CTRL0_DFS32_FRAME_08BITS | | ||
| spi_ctrl0_spi_scph(0x1); | ||
| val = SPI_CTRL0_TMOD_EEPROM_READ | SPI_CTRL0_SPI_FRF_STANDARD | SPI_CTRL0_DFS32_FRAME_08BITS; |
blozano-tt
reviewed
Feb 10, 2026
|
|
||
| val = SPI_CTRL0_TMOD_TRANSMIT_ONLY | SPI_CTRL0_SPI_FRF_STANDARD | SPI_CTRL0_DFS32_FRAME_08BITS | | ||
| spi_ctrl0_spi_scph(0x1); | ||
| val = SPI_CTRL0_TMOD_TRANSMIT_ONLY | SPI_CTRL0_SPI_FRF_STANDARD | SPI_CTRL0_DFS32_FRAME_08BITS; |
Contributor
|
I'm fine to merge as long as spi tests pass. These need to be ran on a dedicated machine. @pjanevskiTT if you have the time, please run the tests on one of the wh machines (this is wh specific code) |
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
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
Fixing clang-tidy checker:
misc-redundant-expressionAnalysis run: #21555444024
Status
🔄 Copilot is reviewing remaining violations...
Generated by clang-tidy autofix workflow