search

ssokolow / nodo

Pre-emptively created repository so the design can be discussed on the issue tracker before commits are made (repo name may change)
Apache License 2.0
18 stars 0 forks source link

Use of "BTreeMap" in src/config.rs is immature optimization #3

Closed NobodyXu closed 2 years ago

NobodyXu commented 2 years ago

I believe use of BTreeMap in src/config.rs is an immature optimization, since Hashmap should be more than enough for this scenario.

ssokolow commented 2 years ago

It's not an optimization. Note the comment on the use:

use std::collections::BTreeMap; // Used to preserve key ordering in Debug output
NobodyXu commented 2 years ago

Sorry I didn't notice that.

ssokolow commented 2 years ago

That said, that may not be quite accurate.

I'm used to manually keeping my keys alphabetized and I think, since I wrote that snippet, I discovered that it's actually enforcing a sort order which just happens to be what I was doing by hand.

I'll need to double-check that but, either way, it ensures the Debug output is more deterministic.

EDIT: Yeah. I've pushed a correction to the comment.

ssokolow commented 2 years ago

...but you did remind me of something I'd have forgotten to use if I chose to derive(Serialize).

TOML requires you to #[serde(serialize_with = "toml::ser::tables_last")] your maps to reconcile Serde's behaviour with one of TOML's more unusual validity requirements.