derive: unify filter argument handling #318

Kijewski · 2025-01-21T02:04:46Z

No description provided.

GuillaumeGomez · 2025-01-21T09:52:58Z

rinja_derive/src/generator/expr.rs

+            "unexpected argument{} in `{name}` filter",
+            if args.len() == 2 { "" } else { "s" },


Suggested change

"unexpected argument{} in `{name}` filter",

if args.len() == 2 { "" } else { "s" },

"expected 1 argument in `{name}` filter, found {}",

args.len(),

Better than handling plural: not handling plural. :D

Good idea, but the "1" is not true in all cases. :) I reworded the message, and implemented it with less magic. 🪄

GuillaumeGomez · 2025-01-23T13:43:53Z

Conflict. I'll approve once updated otherwise it doesn't work. :')

Kijewski · 2025-01-23T22:42:39Z

Would conflict with #311, too. I'll rebase it after #311 is merged. :)

Kijewski · 2025-01-24T23:02:32Z

I expanded slightly on the initial idea.

Kijewski · 2025-01-24T23:09:27Z

rinja_derive/src/generator/expr.rs

-        if BUILTIN_FILTERS_NEED_ALLOC.contains(&name) {
-            ensure_filter_has_feature_alloc(ctx, name, node)?;
-        }


Check was in the wrong method. Moved to _visit_builtin_filter.

Kijewski · 2025-01-24T23:09:54Z

rinja_derive/src/generator/expr.rs

@@ -303,43 +297,18 @@ impl<'a> Generator<'a, '_> {
        buf: &mut Buffer,
        name: &str,
        args: &[WithSpan<'_, Expr<'a>>],
-        generics: &[WithSpan<'_, TyGenerics<'_>>],


No built-in filter needs generics.

GuillaumeGomez · 2025-01-25T00:13:52Z

rinja_derive/src/generator/expr.rs

-                return self._visit_builtin_filter(ctx, buf, name, args, generics, node);
+            "urlencode" | "urlencode_strict" => Self::_visit_urlencode_filter,
+            name if BUILTIN_FILTERS.contains(&name) => Self::_visit_builtin_filter,
+            "value" => {


Why moving "value" below the name check?

You still didn't answer to this one. 😉

I moved the filters that return to the bottom, to make it easier to follow the code. In the last/current implementation it's strictly alphabetical again. :)

In this case I don't think it's the right way to do it. In the (unlikely) case we added (by mistake) value into BUILTIN_FILTERS, then we'd get no way of noticing that the real value filter is not called until the tests failed.

But anyway, in the current case I think the code can be improved a bit (adding suggestion comment below). 😛

GuillaumeGomez · 2025-01-25T00:16:21Z

rinja_derive/src/generator/expr.rs

-            "linebreaks" => Self::_visit_linebreaks_filter,
-            "linebreaksbr" => Self::_visit_linebreaksbr_filter,
-            "paragraphbreaks" => Self::_visit_paragraphbreaks_filter,
+            "linebreaks" | "linebreaksbr" | "paragraphbreaks" => Self::_visit_linebreaks_filters,


So we're back giving the filter name as argument? That's one extra argument we could skip but if you think it's better this way, then let's go with it.

GuillaumeGomez · 2025-01-25T00:18:21Z

rinja_derive/src/generator/expr.rs

-            _ => {
-                return self._visit_custom_filter(ctx, buf, name, args, generics);
-            }
+        type Filter = for<'a> fn(


Considering the amount of code it adds, what's the perf gain?

Kijewski · 2025-01-25T02:08:10Z

$ cargo bench --bench filters
noop                    time:   [44.123 µs 44.205 µs 44.283 µs]
                        change: [+0.8450% +1.2321% +1.6350%] (p = 0.00 < 0.05)
                        Change within noise threshold.

no_filters              time:   [51.914 µs 52.539 µs 53.235 µs]
                        change: [+0.0880% +0.9181% +1.8833%] (p = 0.05 < 0.05)
                        Change within noise threshold.

few_filters             time:   [76.035 µs 76.535 µs 76.984 µs]
                        change: [+1.2029% +1.8326% +2.4187%] (p = 0.00 < 0.05)
                        Performance has regressed.

all_filters             time:   [96.863 µs 97.574 µs 98.471 µs]
                        change: [-8.9618% -8.4126% -7.7142%] (p = 0.00 < 0.05)
                        Performance has improved.

some_filters_twice      time:   [70.936 µs 71.177 µs 71.383 µs]
                        change: [-8.5514% -8.1406% -7.7478%] (p = 0.00 < 0.05)
                        Performance has improved.

The first two tests are only to measure that my CPU is probably running hotter on the second run. (It shows +216.0°C, but I'm fairly confident that the measurement is incorrect. :D)

GuillaumeGomez · 2025-01-25T15:53:45Z

I'm shared here: the code complexity increases a lot, and although % improvement is noticeable, we're talking in µs. I don't think it's noticeable for a user and therefore, I don't think it's worth the extra complexity.

Kijewski · 2025-01-25T17:48:46Z

Back to a simple match. The timings are actually about the same.

GuillaumeGomez · 2025-01-26T19:27:32Z

rinja_derive/src/generator/expr.rs

+            name if BUILTIN_FILTERS.contains(&name) => Self::_visit_builtin_filter,
+            "value" => {
+                return self._visit_value(ctx, buf, args, generics, node, "`value` filter");
+            }
+            _ => {
+                return self._visit_custom_filter(ctx, buf, name, args, generics);
            }


Suggested change

name if BUILTIN_FILTERS.contains(&name) => Self::_visit_builtin_filter,

"value" => {

return self._visit_value(ctx, buf, args, generics, node, "`value` filter");

}

_ => {

return self._visit_custom_filter(ctx, buf, name, args, generics);

}

"value" => {

return self._visit_value(ctx, buf, args, generics, node, "`value` filter");

}

name => if BUILTIN_FILTERS.contains(&name) {

Self::_visit_builtin_filter

} else {

return self._visit_custom_filter(ctx, buf, name, args, generics);

},

This allows to remove one arm and have the contains call to be done after all other strings have been checked.

GuillaumeGomez reviewed Jan 21, 2025

View reviewed changes

Kijewski force-pushed the pr-unify-filter-arg branch from 98e1791 to 6a95aee Compare January 21, 2025 18:39

Kijewski force-pushed the pr-unify-filter-arg branch 2 times, most recently from b4dfbbe to a9166d3 Compare January 24, 2025 22:57

Kijewski commented Jan 24, 2025

View reviewed changes

GuillaumeGomez reviewed Jan 25, 2025

View reviewed changes

Kijewski and others added 3 commits January 25, 2025 18:42

Add filters benchmark

2137a6f

derive: unify filter argument handling

ed6d288

derive: move filters into its own file

6ac3ac0

Kijewski force-pushed the pr-unify-filter-arg branch from e611aa3 to 6ac3ac0 Compare January 25, 2025 17:43

GuillaumeGomez reviewed Jan 26, 2025

View reviewed changes

Kijewski added the tbd in v0.13.1 label Feb 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

derive: unify filter argument handling #318

derive: unify filter argument handling #318

Kijewski commented Jan 21, 2025

GuillaumeGomez Jan 21, 2025

Kijewski Jan 21, 2025

GuillaumeGomez commented Jan 23, 2025

Kijewski commented Jan 23, 2025

Kijewski commented Jan 24, 2025

Kijewski Jan 24, 2025

Kijewski Jan 24, 2025

GuillaumeGomez Jan 25, 2025

GuillaumeGomez Jan 25, 2025

Kijewski Jan 26, 2025

GuillaumeGomez Jan 26, 2025

GuillaumeGomez Jan 25, 2025 •

edited

Loading

GuillaumeGomez Jan 25, 2025

Kijewski commented Jan 25, 2025

GuillaumeGomez commented Jan 25, 2025

Kijewski commented Jan 25, 2025

GuillaumeGomez Jan 26, 2025

		"unexpected argument{} in `{name}` filter",
		if args.len() == 2 { "" } else { "s" },

derive: unify filter argument handling #318

Are you sure you want to change the base?

derive: unify filter argument handling #318

Conversation

Kijewski commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 23, 2025

Kijewski commented Jan 23, 2025

Kijewski commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kijewski commented Jan 25, 2025

GuillaumeGomez commented Jan 25, 2025

Kijewski commented Jan 25, 2025

Choose a reason for hiding this comment

GuillaumeGomez Jan 25, 2025 •

edited

Loading