From d7d19b87efe8abb1fbc521cc43cead0c64f30932 Mon Sep 17 00:00:00 2001 From: Rui Marques Date: Wed, 25 Feb 2026 09:36:46 +0000 Subject: [PATCH] Split the quality part of PWR0{28,30} to PWR086 --- Benchmark/src/CMakeLists.txt | 1 + Benchmark/src/PWR086.cpp | 46 +++++++++++++++++++++ Checks/PWR028/README.md | 2 +- Checks/PWR030/README.md | 2 +- Checks/PWR086/README.md | 78 ++++++++++++++++++++++++++++++++++++ Checks/PWR086/example.c | 11 +++++ Checks/PWR086/solution.c | 9 +++++ README.md | 5 ++- 8 files changed, 150 insertions(+), 4 deletions(-) create mode 100644 Benchmark/src/PWR086.cpp create mode 100644 Checks/PWR086/README.md create mode 100644 Checks/PWR086/example.c create mode 100644 Checks/PWR086/solution.c diff --git a/Benchmark/src/CMakeLists.txt b/Benchmark/src/CMakeLists.txt index 001e725b..e4391b0d 100644 --- a/Benchmark/src/CMakeLists.txt +++ b/Benchmark/src/CMakeLists.txt @@ -23,4 +23,5 @@ add_benchmark(PWR080) add_benchmark(PWR081) add_benchmark(PWR082) add_benchmark(PWR083) +add_benchmark(PWR086) add_benchmark(PWR087) diff --git a/Benchmark/src/PWR086.cpp b/Benchmark/src/PWR086.cpp new file mode 100644 index 00000000..24b36373 --- /dev/null +++ b/Benchmark/src/PWR086.cpp @@ -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(N_VEC); + auto b = OpenCatalog::CreateRandomVector(N_VEC); + auto c = OpenCatalog::CreateZeroVector(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(N_VEC); + auto b = OpenCatalog::CreateRandomVector(N_VEC); + auto c = OpenCatalog::CreateZeroVector(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 diff --git a/Checks/PWR028/README.md b/Checks/PWR028/README.md index a1f4aa7c..9d9b09d0 100644 --- a/Checks/PWR028/README.md +++ b/Checks/PWR028/README.md @@ -63,7 +63,7 @@ void example(float *a, float *b, float *c, unsigned size, unsigned inc) { > less error-prone than pointer-based notation. This is especially relevant > when accessing multi-dimensional arrays with a single pointer `*`, where > offsets must be manually computed by multiplying indices with the -> corresponding dimensions. +> corresponding dimensions. See [PWR086](../PWR086/README.md) ### Related resources diff --git a/Checks/PWR030/README.md b/Checks/PWR030/README.md index 0003a84f..dc6d4e69 100644 --- a/Checks/PWR030/README.md +++ b/Checks/PWR030/README.md @@ -69,7 +69,7 @@ for (int i = 0; i < size; i++) { > less error-prone than pointer-based notation. This is especially relevant > when accessing multi-dimensional arrays with a single pointer `*`, where > offsets must be manually computed by multiplying indices with the -> corresponding dimensions. +> corresponding dimensions. See [PWR086](../PWR086/README.md) ### References diff --git a/Checks/PWR086/README.md b/Checks/PWR086/README.md new file mode 100644 index 00000000..166fe95f --- /dev/null +++ b/Checks/PWR086/README.md @@ -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) diff --git a/Checks/PWR086/example.c b/Checks/PWR086/example.c new file mode 100644 index 00000000..9a6c0085 --- /dev/null +++ b/Checks/PWR086/example.c @@ -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; + } +} diff --git a/Checks/PWR086/solution.c b/Checks/PWR086/solution.c new file mode 100644 index 00000000..b15be75a --- /dev/null +++ b/Checks/PWR086/solution.c @@ -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)]); + } +} diff --git a/README.md b/README.md index 66e6580c..7f322877 100644 --- a/README.md +++ b/README.md @@ -56,9 +56,9 @@ designed to demonstrate: | [PWR025](Checks/PWR025/) | Consider annotating pure function with OpenMP 'declare simd' | optimization | | | | | ✓ | ✓ | ✓ | | | [PWR026](Checks/PWR026/) | Annotate function for OpenMP Offload | optimization | | | | | ✓ | ✓ | ✓ | | | [PWR027](Checks/PWR027/) | Annotate function for OpenACC Offload | optimization | | | | | ✓ | ✓ | ✓ | | -| [PWR028](Checks/PWR028/) | Remove pointer increment preventing performance optimization | security, optimization | [CWE-468](https://cwe.mitre.org/data/definitions/468.html) | | [EXP08-C](https://wiki.sei.cmu.edu/confluence/display/c/EXP08-C.+Ensure+pointer+arithmetic+is+used+correctly) | | ✓ | | ✓ | | +| [PWR028](Checks/PWR028/) | Remove pointer increment preventing performance optimization | optimization | | | | | ✓ | | ✓ | | | [PWR029](Checks/PWR029/) | Remove integer increment preventing performance optimization | optimization | | | | | ✓ | ✓ | ✓ | | -| [PWR030](Checks/PWR030/) | Remove pointer assignment preventing performance optimization for perfectly nested loops | security, optimization | [CWE-468](https://cwe.mitre.org/data/definitions/468.html) | | [EXP08-C](https://wiki.sei.cmu.edu/confluence/display/c/EXP08-C.+Ensure+pointer+arithmetic+is+used+correctly) | | ✓ | ✓ | ✓ | | +| [PWR030](Checks/PWR030/) | Remove pointer assignment preventing performance optimization for perfectly nested loops | optimization | | | | | ✓ | ✓ | ✓ | | | [PWR031](Checks/PWR031/) | Replace pow by multiplication, division and/or square root | optimization | | | | | ✓ | ✓ | ✓ | | | [PWR032](Checks/PWR032/) | Avoid calls to mathematical functions with higher precision than required | optimization | | | | | ✓ | | ✓ | | | [PWR034](Checks/PWR034/) | Avoid strided array access to improve performance | optimization | | | | | ✓ | ✓ | ✓ | | @@ -99,6 +99,7 @@ designed to demonstrate: | [PWR082](Checks/PWR082/) | Remove unused variables | correctness, security | [CWE-563](https://cwe.mitre.org/data/definitions/563.html) | [6.19](https://j3-fortran.org/doc/year/23/23-241.pdf) | [MSC13-C](https://wiki.sei.cmu.edu/confluence/spaces/c/pages/87152095/MSC13-C.+Detect+and+remove+unused+values) | | ✓ | ✓ | ✓ | | | [PWR083](Checks/PWR083/) | Match the types of dummy and actual arguments in procedure calls | correctness, security | [CWE-628](https://cwe.mitre.org/data/definitions/628.html), [CWE-758](https://cwe.mitre.org/data/definitions/758.html) | [6.11](https://j3-fortran.org/doc/year/23/23-241.pdf), [6.32](https://j3-fortran.org/doc/year/23/23-241.pdf), [6.53](https://j3-fortran.org/doc/year/23/23-241.pdf), [6.56](https://j3-fortran.org/doc/year/23/23-241.pdf) | [EXP37-C](https://wiki.sei.cmu.edu/confluence/display/c/EXP37-C.+Call+functions+with+the+correct+number+and+type+of+arguments), [MSC15-C](https://wiki.sei.cmu.edu/confluence/display/c/MSC15-C.+Do+not+depend+on+undefined+behavior) | | | ✓ | | | | [PWR085](Checks/PWR085/) | Favor iterative implementations over recursion to prevent stack overflows | security | [CWE-674](https://cwe.mitre.org/data/definitions/674.html) | [6.35](https://j3-fortran.org/doc/year/23/23-241.pdf), [6.43](https://j3-fortran.org/doc/year/23/23-241.pdf) | | | ✓ | ✓ | ✓ | | +| [PWR086](Checks/PWR086/) | Prefer array-based notation over pointer-based notation for readability | security | [CWE-468](https://cwe.mitre.org/data/definitions/468.html) | | [EXP08-C](https://wiki.sei.cmu.edu/confluence/display/c/EXP08-C.+Ensure+pointer+arithmetic+is+used+correctly) | | ✓ | | ✓ | | | [PWR087](Checks/PWR087/) | Declare array dummy arguments as assumed-shape arrays to favor compiler optimizations | optimization | | | | | | ✓ | | | | [PWR088](Checks/PWR088/) | Add missing arguments to procedure calls | correctness, security | [CWE-628](https://cwe.mitre.org/data/definitions/628.html), [CWE-758](https://cwe.mitre.org/data/definitions/758.html) | [6.32](https://j3-fortran.org/doc/year/23/23-241.pdf), [6.56](https://j3-fortran.org/doc/year/23/23-241.pdf) | [EXP37-C](https://wiki.sei.cmu.edu/confluence/display/c/EXP37-C.+Call+functions+with+the+correct+number+and+type+of+arguments), [MSC15-C](https://wiki.sei.cmu.edu/confluence/display/c/MSC15-C.+Do+not+depend+on+undefined+behavior) | | | ✓ | | | | [PWR089](Checks/PWR089/) | Remove unexpected arguments from procedure calls | correctness, security | [CWE-628](https://cwe.mitre.org/data/definitions/628.html), [CWE-758](https://cwe.mitre.org/data/definitions/758.html) | [6.32](https://j3-fortran.org/doc/year/23/23-241.pdf), [6.56](https://j3-fortran.org/doc/year/23/23-241.pdf) | [EXP37-C](https://wiki.sei.cmu.edu/confluence/display/c/EXP37-C.+Call+functions+with+the+correct+number+and+type+of+arguments), [MSC15-C](https://wiki.sei.cmu.edu/confluence/display/c/MSC15-C.+Do+not+depend+on+undefined+behavior) | | | ✓ | | |