Json method cleanup #12838

zslayton · 2014-03-12T03:11:28Z

Names of is_str and as_str changed to is_string and as_string in order to be consistent with Json enum variants.
Comment fixed.
find_path reworked to be return None as early as possible if no value is found on the provided path. Edit: Informal benchmark: source, results
Impls of is_* methods revised to be more concisely expressed in terms of their respective as_* methods.

alexcrichton · 2014-03-12T03:25:27Z

src/libserialize/json.rs

+                return None;
+            }
+        }
+        target


Did you verify that this was indeed faster? I would expect LLVM to possibly perform an optimization similar to this.

Perhaps I've underestimated LLVM. I didn't think it would be able to infer that find() would always return None following a failure.

I'll put together a benchmark tomorrow evening and share my results.

I ran some tests this morning comparing the for loop version and the fold version. The source can be found here.

Results:

fold version, compiled using rustc:

real 0m0.168s user 0m0.164s sys 0m0.004s real 0m0.169s user 0m0.164s sys 0m0.004s real 0m0.169s user 0m0.160s sys 0m0.004s

for loop version, compiled using rustc:

real 0m0.053s user 0m0.048s sys 0m0.000s real 0m0.051s user 0m0.048s sys 0m0.000s real 0m0.051s user 0m0.048s sys 0m0.000s

fold version, compiled using rustc --opt-level 3:

real 0m0.013s user 0m0.012s sys 0m0.000s real 0m0.011s user 0m0.004s sys 0m0.004s real 0m0.012s user 0m0.004s sys 0m0.004s

for loop version, compiled using rustc --opt-level 3:

real 0m0.005s user 0m0.000s sys 0m0.004s real 0m0.005s user 0m0.000s sys 0m0.008s real 0m0.005s user 0m0.000s sys 0m0.004s

The numbers don't lie! You may want to restructure this slightly with fewer options:

pub fn find_path<'a>(&'a self, keys: &[&~str]) -> Option<&'a Json>{ // early return so we don't keep searching let mut target = self; for key in keys.iter() { match target.find(*key) { Some(t) => { target = t; } None => return None, } } Some(target) }

…mized, method impls refactored to reduce repitition. Fixed formatting, reworked find_path to use fewer Options. Removed stray tab.

…ing-assignment, r=Veykril Fix missing fields check on destructuring assignment Fixes rust-lang#12838 When checking if the record literal in question is an assignee expression or not, the new fn `is_assignee_record_literal` iterates over its ancestors until it is sure. This isn't super efficient, as we don't cache anything and does the iteration for every record literal during missing fields check. Alternatively, we may want to have a field like `assignee` on `hir_def::Expr::{RecordLit, Array, Tuple, Call}` to tell if it's an assignee expression, which would be O(1) when checking later but have some memory overhead for the field.

For restriction lints, replace “Why is this bad?” with “Why restrict this?” The `restriction` group contains many lints which are not about necessarily “bad” things, but style choices — perhaps even style choices which contradict conventional Rust style — or are otherwise very situational. This results in silly wording like “Why is this bad? It isn't, but ...”, which I’ve seen confuse and distress a newcomer at least once. To improve this situation, this PR replaces the “Why is this bad?” section heading with “Why restrict this?”, for most, but not all, restriction lints. I left alone the ones whose placement in the restriction group is more incidental. In order to make this make sense, I had to remove the “It isn't, but” texts from the contents of the sections. Sometimes further changes were needed, or there were obvious fixes to make, and I went ahead and made those changes without attempting to split them into another commit, even though many of them are not strictly necessary for the “Why restrict this?” project; it seemed to me that it was more valuable to grab the low-hanging fruit than to be careful about it. changelog: rephrased the documentation of `restriction` lints for clarity about their nature

alexcrichton reviewed Mar 12, 2014
View reviewed changes

Closes rust-lang#12829. Names changed for consistency, find_path opti…

b828ba9

…mized, method impls refactored to reduce repitition. Fixed formatting, reworked find_path to use fewer Options. Removed stray tab.

alexcrichton closed this in 3f2434e Mar 12, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Json method cleanup #12838

Json method cleanup #12838

zslayton commented Mar 12, 2014

alexcrichton Mar 12, 2014

zslayton Mar 12, 2014

zslayton Mar 12, 2014

alexcrichton Mar 12, 2014

Json method cleanup #12838

Json method cleanup #12838

Conversation

zslayton commented Mar 12, 2014

alexcrichton Mar 12, 2014

Choose a reason for hiding this comment

zslayton Mar 12, 2014

Choose a reason for hiding this comment

zslayton Mar 12, 2014

Choose a reason for hiding this comment

alexcrichton Mar 12, 2014

Choose a reason for hiding this comment