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

Additional headers for easy-to-use light(er,est) entry points #1191

Merged
merged 3 commits into from
Dec 10, 2021

Conversation

eddelbuettel
Copy link
Member

This PR adds three new headers (along with a new alternate Rcpp/Rcpp pulling in Rcpp.h) to offer (hopefully) easier access to (simple) speedups from unused headers / features.

An older work-in-in-progress branch, renamed featires/new_headers_initial, has the original commits as well as two more files which I do not think add value, hence this new (smaller) branch.

As new entry points, no existing code is changed or affected.

I had been sitting on this for months, undecided, but when I mentioned it recently @kevinushey was in favour so there it for review.

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

@kevinushey
Copy link
Contributor

IMHO rather than having 3 separate headers we should just have a single header RcppLite.h (or similar) that defines RCPP_NO_MODULES + RCPP_NO_SUGAR, as those are the heaviest optional pieces of Rcpp.

If we keep this scheme, I think RTTI is actually less heavyweight (for compile time, at least) than sugar.

Copy link
Member

@Enchufa2 Enchufa2 left a comment

Choose a reason for hiding this comment

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

IMHO rather than having 3 separate headers we should just have a single header RcppLite.h (or similar) that defines RCPP_NO_MODULES + RCPP_NO_SUGAR, as those are the heaviest optional pieces of Rcpp.

But some Rcpp sugar is so convenient. :)

As I said previously, I'm not sure what's the best way to address this, so if you both think this is the best option, then go ahead. Anyway these are my thoughts in no particular order:

  • I think the important thing here is discoverability. There's a FAQ point about this which briefly mentions how to disable modules, mentions RTTI without explanation, and doesn't mention sugar. I don't think many Rcpp users ever read that, and of those who did, I would bet they didn't know what's the meaning nor the implications.
  • That said, I don't see how definining new headers inside the Rcpp folder, next to many others, improves discoverability, and much less understanding of the implications. I agree that includes may be nicer than defines, but the good thing about the current defines is that they say explicitly what they're doing (i.e., no modules, no sugar, and even no RTTI, which I don't think many Rcpp users knows what this means). On the other hand, Light, Lighter, Lightest is much more obscure.
  • Another option (instead of Light, Lighter and Lightest) would be to define Base as no modules + no RTTI, and Minimal as Base + no sugar. How many users would want RTTI but no modules? Maybe add RCPP_ENABLE_RTTI for them? Anyway, I wouldn't define a header for RTTI, because, unlike modules and sugar, it really is a flag.
  • Regardless what we do here, I'm skeptical about general adoption. Rcpp is very well established, and there are sooo many resources, packages and examples out there with just #include <Rcpp.h>. Another approach would be to target disabling those by default in a future release, and create headers to enable modules and sugar. Then contact the maintainers using those features to explicitly enable them with new defines. But probably you've got enough with the strict headers stuff. :)

@eddelbuettel
Copy link
Member Author

IMHO rather than having 3 separate headers we should just have a single header

That's exactly how I started (see the feature/new_headers_initial branch and its initial (of four) commits) and I ended up disliking the name I picked ("RcppFast", and "RcppLite" is also not great). I now truly prefer the finer-grained approach.

As for effect, they all matter a little as I measure, but none comes close to the effect of "throwing it all out" (I have a package for that) and even that is strongly dominated by just using ccache in front. And net-net I think more headers == more choice == more clearly stating there are different #define to set helps.

@eddelbuettel
Copy link
Member Author

eddelbuettel commented Dec 9, 2021

I don't see how definining new headers inside the Rcpp folder, next to many others, improves discoverability

The key is a new entry point Rcpp/Rcpp so that

  • (maybe "eventually") we could even remove the old Rcpp.h a level above
  • I really want to remove the RcppCommon.h a level above
  • with these new entry points I want to clean up Rcpp/* which is sprawling mess

But we need something to support it during the (slow, long) passage to a better layout. This could be a first step.

And I am with you that realistically it will be hard / impossible to move away from Rcpp.h. But Rcpp.h could just call Rcpp/Rcpp (and we get effective change, as well as a different header for new projects, plus some choice) and the directory below could be much different with many of the way more internal headers tucked away more cleanly.

@eddelbuettel
Copy link
Member Author

Assuming / hoping this is not to offensive to @kevinushey I plan to merge this today given the nod from @Enchufa2.

@kevinushey
Copy link
Contributor

LGTM! I still prefer a single separate "light" header but don't feel that strongly either way.

@eddelbuettel
Copy link
Member Author

Sounds good -- We can treat it as incremental work in progress.

@eddelbuettel eddelbuettel merged commit d0ef311 into master Dec 10, 2021
@eddelbuettel eddelbuettel deleted the feature/new_headers branch December 10, 2021 18:05
@eddelbuettel
Copy link
Member Author

Any use in keeping the predecessor 'feature/new_headers_initial' branch around? Probably not but I figure I might ask before blowing it out.

@Enchufa2
Copy link
Member

No need here.

@eddelbuettel
Copy link
Member Author

I also prefer clean and sparse so it'll likely be gone soon.

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.

None yet

3 participants