Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use i64::MAX instead of 0x3f3f3f3f3f3f3f3f #22

Merged
merged 1 commit into from Aug 14, 2021
Merged

Conversation

dllu
Copy link
Contributor

@dllu dllu commented Aug 12, 2021

seems more rusty to use built-in constants

it seems that 0x3f3f... is used in competitive programming because...

  1. it is around 2^62 so you can double it without overflowing i64. However it doesn't seem to matter here.
  2. it is easy to type and remember, especially in c++ where you have to include <numeric_limits> and use std::numeric_limits<int64_t>::max(). But in rust i64::MAX is easier to type.
  3. you can do memset(..., 0x3f, ...) tricks. But here we are initializing with vec![Self::INF; self.graph.num_v()] which should have compiler optimizations anyway.

tests seem to pass...

@EbTech
Copy link
Owner

EbTech commented Aug 12, 2021

Hmm you're right that the constant here is pretty arbitrary. That said, given that contestants may need to modify their codebook algorithms on the spot, I am a bit worried about potential uses involving addition, which would induce overflow. To make this risk more explicit, it might make more sense to remove INF and refer to i64::MAX explicitly everywhere. The downside, of course, is that switching to floating points would now involve changes in multiples places. Alternatively, it might make sense to define an overflow-proof INF at a more global scope in the repo. I'm not sure which approach is best...

Edit: I suppose INF = i64::MAX / 2 should also work, considering that it's hard to imagine Rust-using contestants ever employing memset-style hacks. It's slightly less robust than 3f3f if you have a formula like a + b + 2, but it's not like we can be robust to every possible operation anyway. This constant could be defined either here or in some other location, as above. Is it weird to put a /2 if the current code doesn't need it? Your approach has the advantage that all variations, whether floats or /2, can be had with a one-line change.

@EbTech
Copy link
Owner

EbTech commented Aug 12, 2021

Considering that we don't do any arithmetic with the INFs, nor use them in calls to min or max, would it be rustier to represent them with Options? Though that presents three new drawbacks:

  • Not self-documenting
  • Will be inconvenient if either of the above assumptions cease to hold
  • Compiler won't know to optimize out the enum flag

Ok none of the solutions seem ideal, but I'm somewhat inclined to take your version now. Thoughts?

@dllu
Copy link
Contributor Author

dllu commented Aug 12, 2021

hmm Option might be more semantic but then compilers would likely have a hard time optimizing it, for example it won't be clear that SIMD operations would be possible on a vector of options.

I am a bit worried about potential uses involving addition

I guess for this particular mod for graphs, it seems that this is more of a sentinel value and unlikely to be actually used for arithmetic (correct me if I'm wrong here). Perhaps each mod could define its own INF (possibly with a more semantic name than just INF) or similar value based on what it needs to do?

Copy link
Owner

@EbTech EbTech left a comment

Choose a reason for hiding this comment

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

See discussion but looks good, thanks!

@EbTech EbTech merged commit 7b71843 into EbTech:master Aug 14, 2021
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.

None yet

2 participants