Re: [PATCH v3 31/31] build: enable shadowed variable warnings

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon,  1 Dec 2025 11:44:48 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> Enable -Wshadow globally for DPDK builds.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Since this was a big series and hard for humans to review;
ran AI code review on it.

I'll review this DPDK patch series for fixing shadow variable warnings according to the AGENTS.md criteria. This is a 31-patch series from Bruce Richardson that enables `-Wshadow` warnings globally for DPDK.

---

## Patch Series Summary

This series adds helper macros (`RTE_MIN3`/`RTE_MAX3`), fixes shadow variable warnings across libraries, drivers, and apps, then enables `-Wshadow` globally. Components that don't build cleanly are explicitly excluded.

---

## Patch-by-Patch Review

### Patch 1/31: `eal: add more min/max helpers`

**Subject Line:** ✅ Good - 30 characters, correct prefix, lowercase, imperative

**Commit Message:**
- ✅ Describes the change well
- ✅ Proper Signed-off-by tags
- ✅ Has Acked-by tags

**Code Review:**
```c
#define RTE_MIN3(a, b, c)                      \
	__extension__ ({                        \
		typeof (a) _a = (a);            \
		typeof (b) _b = (b);            \
		typeof (c) _c = (c);            \
```

⚠️ **Warning (style):** There's inconsistent spacing. The `RTE_MAX` macro is modified to use `_ax`/`_bx`, but `RTE_MIN3` and `RTE_MAX3` still use `_a`/`_b`/`_c`. This inconsistency could still cause shadowing if `RTE_MIN3(RTE_MIN(...))` is used. Consider using unique names like `_a3`/`_b3`/`_c3` for the triple variants.

⚠️ **Warning (style):** Extra blank line between `RTE_MIN3` and the `RTE_MAX` definition comment:
```c
	})

                   <-- extra blank line
/**
 * Macro to return the minimum of two numbers
```
DPDK style generally avoids double blank lines.

**Missing:** No `Cc: stable@dpdk.org` - this is a new feature, not a bug fix, so that's correct.

---

### Patch 2/31: `eal: fix variable shadowing`

**Subject Line:** ✅ Good - 27 characters

**Commit Message:**
- ✅ Has multiple `Bugzilla ID:` tags (correct format)
- ✅ Has `Fixes:` tags with 12-char SHA
- ✅ Has `Cc: stable@dpdk.org`
- ✅ Proper tag order

**Code Changes:**
- Renaming `trace` to `t` in tracing code - ✅ Reasonable
- Renaming `args` to `out_args` - ✅ Clear intent
- Renaming `optarg` to `arg` and `optlen` to `arglen` - ✅ Avoids conflict with getopt global

**Issue in `drivers/bus/pci/windows/pci.c`:**
```c
	} else {
		struct rte_pci_device *dev2 = NULL;
-		int ret;
```
This removes `ret` but it's used later in the block. Let me verify... Looking at the patch context, `ret` is already defined at the outer scope, so this is correct.

✅ **Good patch**

---

### Patch 3/31: `ethdev: fix variable shadowing issues`

**Subject Line:** ✅ 37 characters, correct prefix (`ethdev:` not `lib/ethdev:`)

**Code:**
```c
-	ethdev_bus_specific_init ethdev_bus_specific_init,
+	ethdev_bus_specific_init bus_specific_init,
```
✅ Renames parameter to avoid shadowing the type name - good practice.

✅ **Good patch**

---

### Patch 4/31: `eventdev: fix variable shadowing issues`

**Subject Line:** ✅ Good

**Code Review:**
```c
#define TXA_CHECK_OR_ERR_RET(id) \
do {\
-	int ret; \
+	int _ret; \
```
✅ Using `_ret` prefix for macro-local variables is the correct pattern.

```c
-	uint32_t n;
...
+			uint32_t n = rxa_eth_rx(rx_adapter, port, queue, nb_rx,
```
✅ Reduced scope by moving declaration to initialization point.

✅ **Good patch**

---

### Patch 5/31: `net: remove shadowed variable`

**Subject Line:** ✅ 30 characters

**Code:**
```c
-	__mmask16 mask;
...
-		mask = byte_len_to_mask_table[data_len];
-		d = _mm_maskz_loadu_epi8(mask, data);
+		d = _mm_maskz_loadu_epi8(byte_len_to_mask_table[data_len], data);
```
✅ Cleanly eliminates unnecessary variable.

✅ **Good patch**

---

### Patch 6/31: `pipeline: fix variable shadowing`

**Subject Line:** ✅ Good

Multiple good fixes:
- Loop counter `i` shadowing → use inline `for (uint32_t j = ...)`  
- Local `n_bytes` renamed to `total_bytes`
- Local `name` renamed contextually (`action_name`, `pipeline_name`)

✅ **Good patch**

---

### Patch 7/31: `table: fix issues with variable shadowing`

**Subject Line:** ✅ Good

**Macro variables renamed with underscore prefix:**
```c
-	uint64_t x, pos, x0, x1, x2, mask;
+	uint64_t _x, _pos, _x0, _x1, _x2, _mask;
```
✅ Correct pattern for macro-local variables.

✅ **Good patch**

---

### Patches 8-10: `power`, `pcapng`, `bbdev` fixes

All ✅ **Good patches** - simple renames, proper format.

---

### Patch 11/31: `bus/pci: remove shadowed variables`

**Subject Line:** ✅ Good

Both fixes are correct - removing duplicate `ret` declarations.

✅ **Good patch**

---

### Patches 12-17: Intel driver fixes (e1000, i40e, ice, cpfl, ixgbe)

All have proper format. Notable:

**Patch 14 (i40e):**
```c
-	struct rte_eth_dev *dev;
+	struct rte_eth_dev *_dev;
```
✅ Underscore prefix in macros is correct.

**Patch 15 (ice):**
```c
-	vsi->nb_qps = RTE_MIN
-		((uint16_t)vsi->adapter->hw.func_caps.common_cap.num_msix_vectors - 2,
-		RTE_MIN(vsi->nb_qps, ICE_MAX_Q_PER_TC));
+	vsi->nb_qps = RTE_MIN(vsi->nb_qps, ICE_MAX_Q_PER_TC);
+	vsi->nb_qps = RTE_MIN(vsi->nb_qps,
+		(uint16_t)vsi->adapter->hw.func_caps.common_cap.num_msix_vectors - 2);
```
✅ Good - avoids nested `RTE_MIN` which causes shadowing.

---

### Patch 18/31: `app/testpmd: fix build with shadow warnings enabled`

**Subject Line:** ✅ 47 characters

Multiple good fixes including:
- `geneve_opt` vs generic `opt`
- `event_arg` vs `optarg` (getopt global)
- Removing unnecessary `rss_hf` variable

✅ **Good patch**

---

### Patches 19-27: Various app fixes

All ✅ **Good patches** with proper format and reasonable changes.

---

### Patch 28/31: `drivers: disable variable shadowing warnings for drivers`

**Subject Line:** ✅ Good

**Code:**
```python
no_shadow_cflag = []
if cc.has_argument('-Wno-shadow')
        no_shadow_cflag = '-Wno-shadow'
endif
```

⚠️ **Info:** This pattern is correct but many drivers are excluded. The list is quite long (33 drivers/components). This is acceptable as a pragmatic approach to enable the warning incrementally.

✅ **Good patch**

---

### Patches 29-30: app/test and examples exclusions

✅ **Good patches** - correctly excludes code that hasn't been fixed yet.

---

### Patch 31/31: `build: enable shadowed variable warnings`

**Subject Line:** ✅ Good

```python
global_cflags = [
        ...
+        '-Wshadow',
```

✅ **Good patch** - final enablement.

---

## Overall Assessment

### Errors (must fix):
None identified.

### Warnings (should fix):

1. **Patch 1:** Inconsistent variable naming between `RTE_MIN`/`RTE_MAX` (`_ax`/`_bx`) and `RTE_MIN3`/`RTE_MAX3` (`_a`/`_b`/`_c`). Using `RTE_MIN3(RTE_MIN(...))` could still cause shadowing. Consider making variable names more unique.

2. **Patch 1:** Extra blank line between macros (minor style issue).

### Info (consider):

1. The series excludes a significant number of drivers and examples from `-Wshadow`. This is a pragmatic approach but creates technical debt. Consider documenting which components need fixing in a TODO or tracking bug.

2. Some patches fix code in areas unrelated to the commit message scope (e.g., patch 2 modifies `drivers/bus/pci/windows/pci.c` but is titled as "eal" fix). This is acceptable since the changes are necessary for clean compilation.

---

## Summary

This is a **well-structured patch series** that methodically enables `-Wshadow` for DPDK. The patches:
- Follow DPDK commit message conventions
- Have proper `Fixes:` and `Cc: stable@dpdk.org` tags where appropriate
- Use reasonable renaming strategies
- Correctly employ underscore prefixes for macro-local variables

**Recommendation:** The series is ready for merge with minor style considerations on patch 1. The approach of enabling the warning globally while excluding problematic components is pragmatic and allows incremental improvement.