Conversation
| %% upstream version: | ||
| %% https://github.com/massemanet/redbug/pull/10 | ||
| , [ {redbug, {git, "https://github.com/robertoaloi/redbug.git", {branch, "relax-beam-lib-error-matching"}}} | ||
| , {eflame, {git, "https://github.com/jfacorro/eflame.git", {branch, "various.improvements"}}} |
There was a problem hiding this comment.
I would prefer to bring in the upstream version, if possible.
What are the included changes?
Are you pull-requesting them to upstream?
There was a problem hiding this comment.
This is the PR with all the improvements.
src/els_parser.erl
Outdated
| {'ok', erl_syntax:forms() | none, integer()} | ||
| | {'eof', integer()} | ||
| | {'error', any(), integer()}. | ||
| parse_form(Dev, L0, Parser, _Options) -> |
There was a problem hiding this comment.
Can we please use the original naming for the variables? E.g. IoDevice instead of Dev, StartLoc instead of L0, Tokens instead of Ts, etc?
src/els_parser.erl
Outdated
| parse_form(Dev, L0, Parser, _Options) -> | ||
| case io:scan_erl_form(Dev, "", L0) of | ||
| {ok, Ts, L1} -> | ||
| case catch {ok, Parser(Ts, undefined)} of |
There was a problem hiding this comment.
Please don't use the deprecated catch. Use try/catch instead.
src/els_parser.erl
Outdated
| case erl_syntax:type(Form) of | ||
| attribute -> | ||
| case erl_syntax_lib:analyze_attribute(Form) of | ||
| case catch erl_syntax_lib:analyze_attribute(Form) of |
There was a problem hiding this comment.
Same here. Please don't use the old-style catch.
src/els_parser.erl
Outdated
| | none, integer()} | ||
| | {'eof', integer()} | ||
| | {'error', any(), integer()}. | ||
| parse_form(Dev, L0, Options) -> |
There was a problem hiding this comment.
This seems a re-implementation of els_dodger:parse_form/3. Why not to expose that one, instead?
There was a problem hiding this comment.
Sorry, I was misreading the code. Ignore this comment.
There was a problem hiding this comment.
An alternative option would have been to add a callback function to the Options list which could have been executed from within the els_dodger, and have the parse_form/4 return the final accumulator together with the list of forms.
There was a problem hiding this comment.
That would have required a change in the implementation of dodger which I was trying to avoid.
src/els_parser.erl
Outdated
| ), | ||
| {ok, Acc} | ||
| {error, _IoErr, _L1} = Err -> Err; | ||
| {error, _Reason} -> {eof, L0}; % This is probably encoding problem |
There was a problem hiding this comment.
I know the (missing an article) comment comes from the dodger, but I would get rid of it and, instead, add a comment to the top of this function explaining this is a simplified version of the respective dodger function.
src/els_parser.erl
Outdated
| fun(T, Acc) -> | ||
| [do_points_of_interest(T)|Acc] | ||
| end, [], Tree)). | ||
| FoldFun = fun(T, Acc) -> do_points_of_interest(T) ++ Acc end, |
There was a problem hiding this comment.
Do we need the ++ in here? Or can we live with nested lists that we can flatten at the end?
There was a problem hiding this comment.
Do you mean at the end as in parse/1 or at the end of the fold? If it's the former then I would agree, if it's the latter, I think it's much clearer what's happening if we use the ++.
src/els_parser.erl
Outdated
| , [EndLoc, ErrorInfo] | ||
| ), | ||
| {ok, Acc} | ||
| {error, _IoErr, _L1} = Err -> Err; |
There was a problem hiding this comment.
Considering we are pattern matching on {ok, ...} above, maybe we can avoid the defensive style altogether here.
There was a problem hiding this comment.
The result from this function is processed by the code in els_dodger which is pattern matching and handling each of these results though.
src/els_parser.erl
Outdated
| {ok, Ts, L1} -> | ||
| case catch {ok, Parser(Ts, undefined)} of | ||
| {ok, F} -> | ||
| POIs = [find_attribute_pois(F, Ts), points_of_interest(F)], |
There was a problem hiding this comment.
Now that we have the single pass, it probably makes more sense to check here whether we are dealing with an attribute and execute the find_attribute_pois function only in that case.
There was a problem hiding this comment.
The check is done in the function itself, adding it here would only make this function harder to read and would bring no benefit IMO.
2 participants
Description
Expose functions from
els_dodgerto be able to make a single pass over all the forms. This improves the performance a bit, although not as much as we would have expected.Fixes #235.
The following are flame graphs captured by running
eflame:apply(els_indexer, index_dir, ["src"]).and then generating the SVG with the following command:Before
After