Opened 8 weeks ago

Closed 5 weeks ago

#26245 closed defect (implemented)

Unused Rust code warnings

Reported by: teor Owned by: catalyst
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.3.4.1-alpha
Severity: Normal Keywords: 034-must, rust
Cc: Actual Points:
Parent ID: Points:
Reviewer: isis Sponsor:

Description

Recent Rust versions (1.26.1) report the following warnings on master:

warning: constant item is never used: `N_DIGEST_ALGORITHMS`
  --> external/crypto_digest.rs:74:1
   |
74 | const N_DIGEST_ALGORITHMS: usize = DIGEST_SHA3_512 as usize + 1;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(dead_code)] on by default

warning: type alias is never used: `smartlist_t`
   --> external/crypto_digest.rs:120:1
    |
120 | type smartlist_t = Stringlist;
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Child Tickets

Change History (12)

comment:1 Changed 8 weeks ago by catalyst

Do we want to make warnings fatal when building Rust? Maybe only if --enable-fatal-warnings is set?

comment:2 in reply to:  1 ; Changed 8 weeks ago by nickm

Replying to catalyst:

Do we want to make warnings fatal when building Rust? Maybe only if --enable-fatal-warnings is set?

+1

comment:3 Changed 7 weeks ago by nickm

Keywords: 034-must added; 034-must-maybe removed

comment:4 Changed 7 weeks ago by isis

Keywords: 034-must-maybe added; 034-must removed

I suspect this is caused by #26267.

comment:5 Changed 7 weeks ago by isis

Keywords: 034-must added; 034-must-maybe removed

Unclobbering the keywords.

comment:6 in reply to:  2 Changed 7 weeks ago by isis

Replying to nickm:

Replying to catalyst:

Do we want to make warnings fatal when building Rust? Maybe only if --enable-fatal-warnings is set?

+1

+2

comment:7 Changed 7 weeks ago by nickm

Owner: set to isis
Status: newassigned

comment:8 Changed 5 weeks ago by catalyst

Owner: changed from isis to catalyst

Patch for --enable-fatal-warnings in https://github.com/tlyu/tor/tree/bug26245

comment:9 Changed 5 weeks ago by catalyst

Status: assignedneeds_review

comment:10 Changed 5 weeks ago by isis

Reviewer: isis

I'll review and add patches for removing the smartlist_t definition and N_DIGEST_ALGORITHMS (which it appears we don't use in the C code either?).

comment:11 Changed 5 weeks ago by isis

Okay, catalyst's code looks good to me. I've added some patches on top of their code in my bug26245 branch to fix the warnings/errors.

Could someone else please review my additional changes?

comment:12 Changed 5 weeks ago by nickm

Resolution: implemented
Status: needs_reviewclosed

lgtm for 0.3.4. Merged, added a changes file, merged forward.

Note: See TracTickets for help on using tickets.