Fix fcase() segfault (#6452) #6451
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @MichaelChirico and the rest of your teammates on |
MichaelChirico
force-pushed
the
09-02-more_descriptive_variable_names_for_fcase_counting_variables
branch
from
84e0932 to
49bc983
Compare
|
Generated via commit 49bc983 Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 11 minutes and 37 seconds Time taken to run |
MichaelChirico
changed the title
| @@ -209,12 +209,12 @@ SEXP fcaseR(SEXP rho, SEXP args) { | |||
| "Note that the default argument must be named explicitly, e.g., default=0"), narg - 2); | |||
| } | |||
| int nprotect=0, l; | |||
| int64_t len0=0, len1=0, len2=0; | |||
| int64_t n_ans=0, n_this_arg=0, n_undecided=0; | |||
There was a problem hiding this comment.
n_undecided sounds good to indicate the count of the unresolved elements/cases. An alternative I can think of is n_pending (as in tracking how many elements are left to be evaluated)
Is n_this_arg meant to represent the length of the thens argument that is being processed in the current iteration? (I only see it being assigned to it at 285 below, so I'm wondering if it could be renamed specific to it)
There was a problem hiding this comment.
Thanks! Yes you're right about n_this_arg. I'm roughly indifferent between n_undecided and n_pending; both are way better than len2 :)
|
this is great thanks for refactoring variable names |
|
Made the mistake of merging #6452 into this branch instead of I'll just merge this branch to master, rename the PR & merge here as if nothing happened :) |
…fcase_counting_variables
MichaelChirico
changed the title
MichaelChirico
deleted the
09-02-more_descriptive_variable_names_for_fcase_counting_variables
branch
No description provided.