Use nonzero_lit - Githubissues

AFLplusplus / LibAFL

Advanced Fuzzing Library - Slot your Fuzzer together in Rust! Scales across cores and machines. For Windows, Android, MacOS, Linux, no_std, ...

Other

2.03k stars 319 forks source link

Use nonzero_lit #2589

Closed tokatoka closed 1 month ago

domenukk commented 1 month ago

I have concerns with the changes:

Adding NonZero to constructors makes them way more ugly to use. We can just check the parameter on creation dynamically. I had this originally and changed it back.
I don't think the extra dependency for nonzero_lit is worth it. It makes the code no more legible and is an extra dependency and extra macro.

tokatoka commented 1 month ago

I don't think the extra dependency for nonzero_lit is worth it. It makes the code no more legible and is an extra dependency and extra macro.

unwrap() is more ugly.

tokatoka commented 1 month ago

The problem of returning error is that it is not helpful for most people. people won't see it unless they have errors_backtrace feature (and 90% people don't know that they even need to activate it to see the error)

tokatoka commented 1 month ago

If you just don't want dependency then i just copy paste these 9 lines into bolts

https://docs.rs/nonzero_lit/latest/src/nonzero_lit/lib.rs.html#170-179

domenukk commented 1 month ago

The problem of returning error is that it is not helpful for most people. people won't see it unless they have errors_backtrace feature (and 90% people don't know that they even need to activate it to see the error)

The error will never trigger, why would anyone create a generator that generates 0 bytes? And then the error still is a meaningful error so they will see what's up even without backtrace

domenukk commented 1 month ago

I don't think the extra dependency for nonzero_lit is worth it. It makes the code no more legible and is an extra dependency and extra macro.

unwrap() is more ugly.

It's the way the standard library does it, I didn't invent the language

tokatoka commented 1 month ago

Adapted the stuff

domenukk commented 1 month ago

I'm not sure how much sense it makes to let users shoot themselves in the foot with 0 sized generators and 0 stacking.
I still don't see why we don't just keep checking this inside the constructor and return an IllegalValue, that is literally what the error is for...
With the nonzero! maybe using NonZero in the API isn't so ugly anymore?

I'll leave the choice up to you, just wanted to voice my opinion one last time, do what you think is best ;)

tokatoka commented 1 month ago

0 size generator is odd but 0 stacking makes sense. because that stacking is about power