Skip to content

Conversation

vthriller
Copy link

@vthriller vthriller commented Aug 28, 2025

Without this, the following results in an error:

fn subnlas_by_kind<'a>(nlas: &'a [u8]) -> HashMap<u16, Vec<&'a [u8]>> {
	let mut out = HashMap::new();
	for n in NlasIterator::new(nlas) {
		let n = n.unwrap();
		let entry: &mut Vec<_> = out.entry(n.kind()).or_default();
		(*entry).push(n.value())
	}
	out
}
error[E0515]: cannot return value referencing local variable `n`
    |
212 |         (*entry).push(n.value())
    |                       - `n` is borrowed here
213 |     }
214 |     out
    |     ^^^ returns a value referencing data owned by the current function

@cathay4t
Copy link
Member

Please include unit test case proving your fix.

@vthriller
Copy link
Author

There you go, also simplified example code a little bit.

@vthriller
Copy link
Author

(and one more force-push to fix typo, sorry about that)

src/nla.rs Outdated

// compile-time test, should not result in compiler complaints regarding
// lifetimes or returning values allegedly owned by this function
#[allow(dead_code)]
Copy link
Member

@cathay4t cathay4t Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting something like:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_foo() {

    }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can craft some inputs-outputs, but the whole point is that behavior doesn't change at all, it's all about lifetimes. OTOH there are no NlasIterator tests at all, so there's no redundant work to be done, might as well hit two birds with one stone.

@vthriller
Copy link
Author

I don't like how this test looks, but at least there's no #[allow(dead_code)] anymore.

@vthriller
Copy link
Author

I think I'm done for now.

While I don't like iter.next().unwrap().unwrap().value(), derive(PartialEq) for DecodeError would not make it easier to read this test in the future, so I stuck with that.

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.

2 participants