Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid structure() #2227

Merged
merged 1 commit into from
Oct 4, 2023
Merged

avoid structure() #2227

merged 1 commit into from
Oct 4, 2023

Conversation

MichaelChirico
Copy link
Collaborator

structure() is IME always slower (admittedly on very small absolute scale) than the alternatives, with little tradeoff of readability (if anything I think using setters is more canonical -> more readable). E.g.

library(microbenchmark)

l <- list()

f1 <- function(l) structure(l, class = c("a", "b"), name = "nm")
f2 <- function(l) {
  class(l) <- c("a", "b")
  attr(l, "name") <- "nm"
  l
}

microbenchmark(times = 1e6, f1(l), f2(l))
# Unit: microseconds
#   expr   min    lq     mean median    uq      max neval cld
#  f1(l) 5.432 5.746 7.478789  5.895 6.280 74773.89 1e+06   b
#  f2(l) 1.469 1.616 2.235726  1.681 1.795 69402.46 1e+06  a 

identical(f1(l), f2(l))
# [1] TRUE

@codecov-commenter
Copy link

Codecov Report

Merging #2227 (7d28f52) into main (9c49c6e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7d28f52 differs from pull request most recent head f6d6810. Consider uploading reports for the commit f6d6810 to get more accurate results

@@           Coverage Diff           @@
##             main    #2227   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         114      114           
  Lines        5173     5175    +2     
=======================================
+ Hits         5155     5157    +2     
  Misses         18       18           
Files Coverage Δ
R/utils.R 99.13% <100.00%> (+0.01%) ⬆️

@MichaelChirico MichaelChirico merged commit 8d420b5 into main Oct 4, 2023
20 checks passed
@MichaelChirico MichaelChirico deleted the structure branch October 4, 2023 16:29
@klmr
Copy link
Contributor

klmr commented Nov 7, 2023

This feels very subjective. For what it’s worth, I find the immutable logic using structure() clearer and thus preferable to the reassignment with replacement functions.

I don’t object to providing the lint (after all, many lints are subjective and they are configurable) but I am surprised that this one is being made part of the default list of undesirable functions, since it does not share the objectively objectionable characteristics of the other functions in that list (such as causing global side effects) — instead, it is an entirely subjective preference as far as I can tell.

(And regarding the huge performance difference, this feels worth filing a bug report to https://bugs.r-project.org/ — the difference should not be that big, or exist at all.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants