Skip to content

Fix ListOf inheritance and compatibility of NamesProxy#1464

Open
Enchufa2 wants to merge 2 commits intomasterfrom
fix/1463
Open

Fix ListOf inheritance and compatibility of NamesProxy#1464
Enchufa2 wants to merge 2 commits intomasterfrom
fix/1463

Conversation

@Enchufa2
Copy link
Copy Markdown
Member

Closes #1463.

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

Copy link
Copy Markdown
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

, public RObjectMethods<T>
: public NamesProxyPolicy<ListOf<T>>
, public AttributeProxyPolicy<ListOf<T>>
, public RObjectMethods<ListOf<T>>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit surprised how this suddenly becomes recursive (in the usual 'curious' manner) but does not require code changes. That seems like a free lunch. I am probably missing something.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here was to make use of CRTP; I think I erred in leaving out the ListOf in the inheritance.

parent.set__(new_vec); // #nocov end
}

Rf_namesgets(parent, Shield<SEXP>(x));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like a great simplification we can do now / could not do then.

Copy link
Copy Markdown
Member

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- two comments inline.

@Enchufa2
Copy link
Copy Markdown
Member Author

Let's wait for a revdep also with this one just in case.

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.

UB in ListOf proxies

3 participants