Skip to content

Phase 1: -Wunused-but-set-variable を潰して -Werror=unused-but-set-variable を有効化 - #47

Merged
thawk105 merged 1 commit into
masterfrom
werror-unused-but-set-variable
May 13, 2026
Merged

Phase 1: -Wunused-but-set-variable を潰して -Werror=unused-but-set-variable を有効化#47
thawk105 merged 1 commit into
masterfrom
werror-unused-but-set-variable

Conversation

@thawk105

Copy link
Copy Markdown
Owner

#43 Phase 1 の続き。-Wmaybe-uninitialized (#44) に続いて、-Wunused-but-set-variable (3 件) を潰し、同 PR で ccbench_add_protocol()-Werror=unused-but-set-variable を追加する。

修正

1. cc/cicada/transaction.cc:362 (TxExecutor::delete_record)

 Tuple* tuple;
-bool rmw;
-rmw = false;
 ReadElement<Tuple> *re;
 re = searchReadSet(s, key);
 if (re) {
-  rmw = true;
   tuple = re->rcdptr_;
 } else {

rmw は false→true で設定されるが後続で全く参照されない。同ファイル冒頭の TxExecutor::update() では rmwwrite_set_.emplace_back(..., rmw)rmw ? OpType::RMW : OpType::UPDATE で本当に使うが、delete_record ではコピペ漏れで使い忘れている。3 行まるごと削除。

2. include/dbomb_deterministic.hh:135, :201

 tx.reconnoiter_begin();
-auto ret = BombWorkload<Tuple,Param>::select_im_by_factory(tx, f_id, product_ids);
+// Reconnoiter pass: this call's job is to fill product_ids. The
+// Status is intentionally discarded — if it aborted, product_ids
+// is just empty or partial and the build_bom_tree loop below
+// propagates the abort.
+(void) BombWorkload<Tuple,Param>::select_im_by_factory(tx, f_id, product_ids);
 ...
-ret = BombWorkload<Tuple,Param>::select_im_by_factory(tx, f_id, verify_products);
+// Verify pass: see comment on the first select_im_by_factory call
+// above — Status is discarded by design, downstream code propagates
+// any abort.
+(void) BombWorkload<Tuple,Param>::select_im_by_factory(tx, f_id, verify_products);

reconnoiter pass / verify pass のどちらも select_im_by_factory の目的は product_ids (resp. verify_products) を埋めることであり、Status は後続の build_bom_tree(...)std::equal(product_ids, verify_products) の比較で間接的に伝播する設計。他の callsite (include/bomb.hh:688 など) と違って明示的な if (stat != Status::OK) return が要らない reconnoiter フェーズ特有の事情なので、(void) キャスト + コメントで意図を明示。

3. cc/oze/include/transaction.hh:626 (TxExecutor::get_read_version_cardinality)

 uint64_t get_read_version_cardinality() {
     TxSet txns;
-    auto itr = read_set_.begin();
     for (auto re : read_set_) {
         txns.emplace(re.txid_);
     }
     return txns.size();
 }

itr を取って捨て、その下の range-based for が read_set_ を直接舐めている。itr は完全に未使用。削除。

CMake

 target_compile_options(${target} PRIVATE
-  -Werror=maybe-uninitialized)
+  -Werror=maybe-uninitialized
+  -Werror=unused-but-set-variable)

Test plan

  • GCC 13cmake -B build-gcc13 -DCMAKE_BUILD_TYPE=Release -DENABLE_SANITIZER=OFF && cmake --build build-gcc13 -j → 34/34 (-Werror=unused-but-set-variable 越え)
  • GCC 11 + Debug + ASan (cmake -B build -DCMAKE_BUILD_TYPE=Debug) → 34/34
  • CI 緑

…-set-variable

#43 Phase 1 のうち -Wunused-but-set-variable (3 件) を潰し、ccbench_add_protocol()
に -Werror=unused-but-set-variable を追加する。GCC 13 で確認、Debug+ASan / Release
ともに 34/34 ビルド成功。

Fixes:

- cc/cicada/transaction.cc:362 (TxExecutor::delete_record)
  `rmw` を false→true で計算するが後続で全く参照されない dead code。
  おそらく同ファイル冒頭の TxExecutor::update() からのコピペ漏れ
  (update では write_set_.emplace_back / OpType::RMW 選択で rmw を
  使うが、delete_record の write_set 追加経路は別)。3 行まるごと削除。

- include/dbomb_deterministic.hh:135, :201
  `select_im_by_factory(...)` の Status 戻り値を `auto ret =` で受けて
  捨てている。reconnoiter pass / verify pass どちらも product_ids を
  埋めるのが目的で、abort 状態は後続の build_bom_tree / std::equal
  経由で伝播する設計なので意図的破棄。`(void)` キャストで意図を明示
  + コメント。他の callsite (bomb.hh:688 等) と違って実際に Status を
  使う必要がないのは reconnoiter フェーズ特有の事情。

- cc/oze/include/transaction.hh:626 (TxExecutor::get_read_version_cardinality)
  `auto itr = read_set_.begin();` と range-based for が二重に書かれて
  おり、itr は完全に未使用。削除。
@thawk105
thawk105 merged commit f7c1746 into master May 13, 2026
2 checks passed
thawk105 added a commit that referenced this pull request May 13, 2026
Promotes -Wunused-variable to error in ccbench_add_protocol() and
clears the 167 GCC 13 hits (unique file:line:col, across 36 source
files) it surfaces. Continues the per-flag rollout for #43.

Two of the hits were real latent bugs and got proper fixes; the rest
are dead-receive locals (remove) or genuinely-discarded return values
(make explicit with `(void)`).

1. include/bomb.hh `get_material_cost()` and the identical copy in
   include/bomb_pessimistic.hh both held the return of `tx.read()` in
   an unused `stat`, then only checked `tx.status_ == aborted` before
   dereferencing `body`. Per CLAUDE.md "tx.read returns Status - check
   it", `*body` is left untouched on WARN_NOT_FOUND, so a missing
   MaterialCostMaster row would have caused a stale-pointer
   `cast_to<MaterialCostMaster>` deref. Added the missing
   `if (stat != Status::OK) return false;` line in both copies.

2. cc/oze/{bomb,ycsb}_oze.cc computed `actual_extime` but never printed
   it, while every other workload binary (silo, mocc, mvto, si, ss2pl,
   ermia, cicada, tictoc) prints the line `"actual_extime:\t<value>"`
   after `ShowOptParameters()`. Added the missing print to keep the
   oze output schema consistent with the rest.

- Dead local variables (`Result &myres = std::ref(...)`,
  `uint64_t epoch_timer_start/stop`, `Status stat;` at the top of
  long functions, `Version *expected`, `SetElement<Tuple>* re/we`,
  `bool isInvisible`, `int num_pages / num_skip_propagate`,
  `uint32_t p_id` shadowing in load loops, `SimpleKey<8> key`
  re-declarations inside loops, `auto n = tx.read_set_.size()`
  snapshots): removed.
- Workload-loop temporaries that materialize a value as a deliberate
  side effect (the YCSB read+touch / write+materialize pattern in
  include/ycsb.hh, and the OrderLine touch in
  include/tpcc/tpcc_tx_orderstatus.hh): annotated `[[maybe_unused]]`.
- Loop induction vars whose only role is N-iterations bookkeeping
  (`for (const auto& k : keys)` in cc/oze/transaction.cc where the
  body operates on the whole container, not on `k`): annotated
  `[[maybe_unused]]`. The redundant inner-loop body is preserved as-is
  to keep this PR scoped to the warning.
- Discarded return values from functions whose side effect is what
  the caller wants (`tx.scan`, `tx.delete_record`,
  `Masstrees[...].remove_value`, `read_internal`,
  `select_im_by_factory`, `select_pc_by_factory`): made explicit with
  `(void)` and a brief comment, mirroring the pattern Phase 1 (#47)
  used for `select_im_by_factory` in BoMB.

The `remove_value` `(void)` cast is duplicated across the
silo/mocc/tictoc/ss2pl/d2pl writePhase DELETE branches because they
all share the same code shape; consolidating them is out of scope
here.

```diff
 target_compile_options(${target} PRIVATE
   -Werror=maybe-uninitialized
   -Werror=unused-but-set-variable
-  -Werror=unused-label)
+  -Werror=unused-label
+  -Werror=unused-variable)
```
thawk105 added a commit that referenced this pull request May 13, 2026
Promotes -Wunused-variable to error in ccbench_add_protocol() and
clears the 167 GCC 13 hits (unique file:line:col, across 36 source
files) it surfaces. Continues the per-flag rollout for #43.

Two of the hits were real latent bugs and got proper fixes; the rest
are dead-receive locals (remove) or genuinely-discarded return values
(make explicit with `(void)`).

1. include/bomb.hh `get_material_cost()` and the identical copy in
   include/bomb_pessimistic.hh both held the return of `tx.read()` in
   an unused `stat`, then only checked `tx.status_ == aborted` before
   dereferencing `body`. Per CLAUDE.md "tx.read returns Status - check
   it", `*body` is left untouched on WARN_NOT_FOUND, so a missing
   MaterialCostMaster row would have caused a stale-pointer
   `cast_to<MaterialCostMaster>` deref. Added the missing
   `if (stat != Status::OK) return false;` line in both copies.

2. cc/oze/{bomb,ycsb}_oze.cc computed `actual_extime` but never printed
   it, while every other workload binary (silo, mocc, mvto, si, ss2pl,
   ermia, cicada, tictoc) prints the line `"actual_extime:\t<value>"`
   after `ShowOptParameters()`. Added the missing print to keep the
   oze output schema consistent with the rest.

- Dead local variables (`Result &myres = std::ref(...)`,
  `uint64_t epoch_timer_start/stop`, `Status stat;` at the top of
  long functions, `Version *expected`, `SetElement<Tuple>* re/we`,
  `bool isInvisible`, `int num_pages / num_skip_propagate`,
  `uint32_t p_id` shadowing in load loops, `SimpleKey<8> key`
  re-declarations inside loops, `auto n = tx.read_set_.size()`
  snapshots): removed.
- Workload-loop temporaries that materialize a value as a deliberate
  side effect (the YCSB read+touch / write+materialize pattern in
  include/ycsb.hh, and the OrderLine touch in
  include/tpcc/tpcc_tx_orderstatus.hh): annotated `[[maybe_unused]]`.
- Loop induction vars whose only role is N-iterations bookkeeping
  (`for (const auto& k : keys)` in cc/oze/transaction.cc where the
  body operates on the whole container, not on `k`): annotated
  `[[maybe_unused]]`. The redundant inner-loop body is preserved as-is
  to keep this PR scoped to the warning.
- Discarded return values from functions whose side effect is what
  the caller wants (`tx.scan`, `tx.delete_record`,
  `Masstrees[...].remove_value`, `read_internal`,
  `select_im_by_factory`, `select_pc_by_factory`): made explicit with
  `(void)` and a brief comment, mirroring the pattern Phase 1 (#47)
  used for `select_im_by_factory` in BoMB.

The `remove_value` `(void)` cast is duplicated across the
silo/mocc/tictoc/ss2pl/d2pl writePhase DELETE branches because they
all share the same code shape; consolidating them is out of scope
here.

```diff
 target_compile_options(${target} PRIVATE
   -Werror=maybe-uninitialized
   -Werror=unused-but-set-variable
-  -Werror=unused-label)
+  -Werror=unused-label
+  -Werror=unused-variable)
```
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.

1 participant