-
Notifications
You must be signed in to change notification settings - Fork 12
Add PWR086 #134
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
Merged
Merged
Add PWR086 #134
Changes from all commits
Commits
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 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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| #include "Benchmark.h" | ||
|
|
||
| // Forward-declare the functions to benchmark | ||
| extern "C" { | ||
| void pointer_increment_example(float *a, float *b, float *c, unsigned size, | ||
| unsigned inc); | ||
| void pointer_increment_improved(float *a, float *b, float *c, unsigned size, | ||
| unsigned inc); | ||
| } | ||
|
|
||
| // Size adjusted to fit execution on micro-seconds | ||
| constexpr unsigned N_VEC = 1024 * 1024; | ||
| constexpr unsigned INC = 1; | ||
|
|
||
| #if OCB_ENABLE_C | ||
|
|
||
| static void CPointerIncrementExampleBench(benchmark::State &state) { | ||
| auto a = OpenCatalog::CreateRandomVector<float>(N_VEC); | ||
| auto b = OpenCatalog::CreateRandomVector<float>(N_VEC); | ||
| auto c = OpenCatalog::CreateZeroVector<float>(1); | ||
| // Point to the last element so that b[-i*inc] stays in bounds | ||
| float *bEnd = b.data() + N_VEC - 1; | ||
| for (auto _ : state) { | ||
| pointer_increment_example(a.data(), bEnd, c.data(), N_VEC, INC); | ||
| benchmark::DoNotOptimize(c); | ||
| } | ||
| } | ||
|
|
||
| static void CPointerIncrementImprovedBench(benchmark::State &state) { | ||
| auto a = OpenCatalog::CreateRandomVector<float>(N_VEC); | ||
| auto b = OpenCatalog::CreateRandomVector<float>(N_VEC); | ||
| auto c = OpenCatalog::CreateZeroVector<float>(1); | ||
| // Point to the last element so that b[-i*inc] stays in bounds | ||
| float *bEnd = b.data() + N_VEC - 1; | ||
| for (auto _ : state) { | ||
| pointer_increment_improved(a.data(), bEnd, c.data(), N_VEC, INC); | ||
| benchmark::DoNotOptimize(c); | ||
| } | ||
| } | ||
|
|
||
| OC_BENCHMARK("PWR086 C Pointer Increment Example", | ||
| CPointerIncrementExampleBench); | ||
| OC_BENCHMARK("PWR086 C Pointer Increment Improved", | ||
| CPointerIncrementImprovedBench); | ||
|
|
||
| #endif |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| # PWR086: Prefer array-based notation over pointer-based notation for readability | ||
|
|
||
| ### Issue | ||
|
|
||
| Using pointer increments or pointer assignments to access array elements reduces | ||
| code readability and is more error-prone compared to equivalent array-based | ||
| (index-based) notation. | ||
|
|
||
| ### Actions | ||
|
|
||
| Replace pointer-based array access patterns with array-based (index-based) | ||
| notation. | ||
|
|
||
| ### Relevance | ||
|
|
||
| Pointer-based notation for accessing array elements is a common pattern in C and | ||
| C++ code. While functionally equivalent to array-based notation, pointer-based | ||
| access introduces unnecessary complexity that harms code quality: | ||
|
|
||
| - **Error-proneness**: pointer arithmetic is a frequent source of off-by-one | ||
| errors and out-of-bounds accesses, especially when accessing multi-dimensional | ||
| arrays with a single pointer `*`, where offsets must be manually computed by | ||
| multiplying indices with the corresponding dimensions. Array-based notation | ||
| makes the intent explicit and reduces the chance of such mistakes. | ||
| - **Readability**: understanding which element of the array is being accessed | ||
| requires mentally tracking the pointer's value across modifications in the | ||
| control flow. With array-based notation, the accessed element is immediately | ||
| clear from the index expression. | ||
| - **Maintainability**: changing access regions is significantly harder when | ||
| pointer arithmetic must be updated in tandem. Array-based notation keeps the | ||
| indexing logic self-contained and localized. | ||
|
|
||
| ### Code example | ||
|
|
||
| The following loop uses a pointer increment to walk through the elements of | ||
| array `b`: | ||
|
|
||
| ```c | ||
| void example(float *a, float *b, float *c, unsigned size, unsigned inc) { | ||
| float *bTemp1 = b; | ||
| for (unsigned i = 0; i < size; i++) { | ||
| c[0] += (a[i] * bTemp1[0]); | ||
| bTemp1 -= inc; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| The pointer `bTemp1` is decremented by `inc` on every iteration, making it | ||
| difficult to see at a glance which element of `b` is accessed in each iteration. | ||
| Replacing the pointer notation with array indexing makes the access pattern | ||
| explicit: | ||
|
|
||
| ```c | ||
| void solution(float *a, float *b, float *c, unsigned size, unsigned inc) { | ||
| for (unsigned i = 0; i < size; i++) { | ||
| c[0] += (a[i] * b[(int)(-i * inc)); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Now the relationship between the loop variable `i` and the accessed element of | ||
| `b` is immediately visible. | ||
|
|
||
| > [!TIP] | ||
| > Array-based notation also facilitates compiler optimizations such as automatic | ||
| > vectorization and loop interchange. See [PWR028](../PWR028/README.md) and | ||
| > [PWR030](../PWR030/README.md) for performance-focused checks covering similar | ||
| > code patterns. | ||
|
|
||
| ### Related resources | ||
|
|
||
| * [PWR086 examples](https://github.com/codee-com/open-catalog/tree/main/Checks/PWR086/) | ||
| * [PWR028: Remove pointer increment preventing performance optimization](../PWR028/README.md) | ||
| * [PWR030: Remove pointer assignment preventing performance optimization for perfectly nested loops](../PWR030/README.md) | ||
|
|
||
| ### References | ||
|
|
||
| * [CWE-468: Incorrect Pointer Scaling](https://cwe.mitre.org/data/definitions/468.html) | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // PWR086: Prefer array-based notation over pointer-based notation for | ||
| // readability | ||
|
|
||
| void pointer_increment_example(float *a, float *b, float *c, unsigned size, | ||
| unsigned inc) { | ||
| float *bTemp1 = b; | ||
| for (unsigned i = 0; i < size; i++) { | ||
| c[0] += (a[i] * bTemp1[0]); | ||
| bTemp1 -= inc; | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| // PWR086: Prefer array-based notation over pointer-based notation for | ||
| // readability | ||
|
|
||
| void pointer_increment_improved(float *a, float *b, float *c, unsigned size, | ||
| unsigned inc) { | ||
| for (unsigned i = 0; i < size; i++) { | ||
| c[0] += (a[i] * b[-(int)(i * inc)]); | ||
| } | ||
| } |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.