Bug 17284 - Patch: optimize name creation in unlist(list(a=1))
Summary: Patch: optimize name creation in unlist(list(a=1))
Status: CLOSED FIXED
Alias: None
Product: R
Classification: Unclassified
Component: Wishlist (show other bugs)
Version: R-devel (trunk)
Hardware: All All
: P5 enhancement
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2017-06-11 07:39 UTC by Suharto Anggono
Modified: 2017-07-22 12:12 UTC (History)
1 user (show)

See Also:


Attachments
Against R devel r72776, bind.c: NewExtractNames (1.11 KB, patch)
2017-06-11 07:39 UTC, Suharto Anggono
Details | Diff
Against R devel r72844, bind.c: NewExtractNames: count first (5.64 KB, patch)
2017-06-23 01:45 UTC, Suharto Anggono
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Suharto Anggono 2017-06-11 07:39:55 UTC
Created attachment 2260 [details]
Against R devel r72776, bind.c: NewExtractNames

From my looking of the code in bind.c in R devel r72776, especially function 'NewExtractNames', for
unlist(list(a=1)) ,
"a1" is put for a name of the answer and is overridden later by "a". For the example, this patch avoids the unnecessary "a1".

This handles recursive=FALSE case or an atomic component where the component has length 1 and doesn't have names and the corresponding name of 'unlist' result is the same as the component name (from the parent list).
Comment 1 Martin Maechler 2017-06-20 13:14:46 UTC
You are right probably  that your patch will do what you say,
however it adds even more code to the
  NewExtractNames()
function (in `bind.c`)  and (I think) is somewhat  misusing the `recurse` variable for that.

I'm not making any final decision here, but I would rather want to find a way to rewrite  NewExtractNames()  such that this "a1", then "a"  assignment does not happen  ``automatically'', i.e., because the new logic of the code would not do this "multiple assignment" at all.

Yes, this is related to PR#17292
https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17292
which also mentions  NewExtractNames()  inefficiencies ...
Comment 2 Suharto Anggono 2017-06-23 01:45:21 UTC
Created attachment 2265 [details]
Against R devel r72844, bind.c: NewExtractNames: count first

This is alternative.
In 'NewCount', counting stops as soon as count reaches 2.
Comment 3 Martin Maechler 2017-06-23 10:08:20 UTC
(In reply to Suharto Anggono from comment #2)
> Created attachment 2265 [details]
> Against R devel r72844, bind.c: NewExtractNames: count first
> 
> This is alternative.
> In 'NewCount', counting stops as soon as count reaches 2.

This looks really nice .. thank you.
in my version I've replaced  'NewCount()' by 'namesCount()' (and added some comments),  but I think this a nice step forward, and indeed also fixing 
PR#17292 (https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17292).

I'll wait for a day or two on others comments and would commit a very slightly edited version of your patch.

Thank you very much, Suharto!
Comment 4 Suharto Anggono 2017-07-22 12:12:50 UTC
Cosmetic: Following the style in function 'Newbase',
    else if (*CHAR(base) && count == 1) {
	ans = base;
    }
can be used instead of
    else if (*CHAR(base) && count == 1) ans = base;
in function 'NewName'.

For historical record,
/* int firstpos; */
can be put in declaration of struct NameData. It is like field 'deparse_level' in struct BindData.