Replaced sparse references to packages across a few existing examples in theRpp Modules section with a pointer to the specific section at beginning.
Introduced a new sub-section about usagingsourceCpp() as an alternative tocxxfuntion() +getDynLib().
Package section reviewed with focus on recommended loading vialoadModule()
- A more practical example has been included
- The section about the deprecated loading was left as-is
- Namespace exports are also covered.
Improvements and harmomization of existing examples in sectionRcpp Modules
- General review to make it easier for a user to follow the examples.
- All examples now assumefx (clearly stated) and explicitly callModule(), to make them close to self-contained.
- IncludeWorld class module in S4 dispatch example for completeness.
- Make sure all examples (with R code) are runnable, and optionally enable running them
  - Non-included chunks creating the assumed relevantfx object based on the C++ code in previous named chunk(s).
  - The document has gained parameterseval andresults in the YAML header to control evaluating the examples on demand (e.g.Knit with parameters...).
  - This is pretty non-intrusive and can be easily removed.
- Running the examples revealed compilation errors with the finalstd::vector example, which I fixed with aworkaround to be discussed.
Fixed a few tyops + batch cosmetic alignment:
- \proglang all the way.
- \textsl{Rcpp modules} everywhere but in their own section.
- Aligned and fixed usage of C++ elements in inline text.

I left a few explicit comments marked asCHECK-PR to have some pointers for the PR discussion.

Copy link

ContributorAuthor

riccardoporreca commentedJul 25, 2019

More on the issue with thestd::vector example.

The following is a minimal example

#include<Rcpp.h>typedef std::vector<double> vec;// Free function wrappers to work around errors// error: call of overloaded// ‘method(const char [7], <unresolved overloaded function type>)’ is ambiguousvoidvec_resize (vec* obj,int n) {    obj->resize(n);}// error: no matching function for call to// ‘Rcpp::class_<std::vector<double> >::method(const char [10], <unresolved overloaded function type>)’voidvec_push_back (vec* obj,double value) {    obj->push_back(value);}RCPP_MODULE(mod_vec) {usingnamespaceRcpp;    class_<vec>("vec")    .constructor<int>()// exposing member functions    .method("resize", &vec::resize)// .method("resize", &vec_resize)    .method("push_back", &vec::push_back)// .method("push_back", &vec_push_back)    ;}

With compilation errors (Ubuntu 18.04, compiler details:g++ -std=gnu++11). This are probably errors introduced with C++11 where bothpush_back andresize are overloaded compared to C++11

voidpush_back (const value_type& val);voidpush_back (value_type&& val);voidresize (size_type n);voidresize (size_type n,const value_type& val);

This does not seem to be supported by.const_method and.nonconst_method inRcpp

Copy link

Member

eddelbuettel commentedJul 25, 2019

Lovely stuff. I will make some time to review. Trying to get some other things out the door first so may take a few days. But first off ... big thanks for this!

coatless reviewed

Jul 26, 2019

View reviewed changes

Copy link

Contributor

coatless left a comment

Choose a reason for hiding this comment

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

This is great. I made a few remarks for when@eddelbuettel looks over it.

vignettes/Rcpp-modules.Rmd OutdatedShow resolvedHide resolved

vignettes/Rcpp-modules.RmdShow resolvedHide resolved

vignettes/Rcpp-modules.Rmd OutdatedShow resolvedHide resolved

vignettes/Rcpp-modules.RmdShow resolvedHide resolved

vignettes/Rcpp-modules.Rmd OutdatedShow resolvedHide resolved

vignettes/Rcpp-modules.Rmd

		into the package namespace:

		```{r}
		loadModule("yada", TRUE)

Copy link

Contributor

Choose a reason for hiding this comment

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

Perhaps switch this to a namespace function call?

Suggested change

	loadModule("yada", TRUE)
	Rcpp::loadModule("yada", TRUE)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

This is surely a valid point, although this approach does not seem to be used across other Rcpp vignettes.

vignettes/Rcpp-modules.Rmd OutdatedShow resolvedHide resolved

vignettes/Rcpp-modules.Rmd Outdated

		```

		```{r, eval=FALSE}
		```{r}

Copy link

Contributor

Choose a reason for hiding this comment

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

I feel like there was also a need to add:

import(methods)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I double-checked using the package produced byRcpp::Rcpp.package.skeleton(), removed the DESCRIPTION and NAMESPACE imports ofmethods and things seem to still work.

Copy link

Member

Choose a reason for hiding this comment

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

Thanks for checking that. I will merge this PR but that whole aspect may be worth looking into in another ticket. This changed over the years, and maybe describing just one simple (working) setup is best.

vignettes/Rcpp-modules.Rmd OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

riccardoporreca commentedJul 26, 2019

@eddelbuettel,@coatless, I was happy to provide my small contribution after being a happy user for quite some time.

Copy link

Member

eddelbuettel commentedJul 28, 2019

This is really good work, and I like it a lot. I didn't get torunning it though.

I think we should work out what we want the sections / questions you labeled as 'CHECK-PR' to say before we merge.

Copy link

Member

eddelbuettel commentedAug 3, 2019•
edited
Loading

I truly apologize for taking so long. Life gets in the way at times.

My verdict is mostly two thumbs way up. I would just ask for one more pass, removing all the CHECK-PR as detailed below.

I may still have overlooked some issues, but my quick notes are below.

PS We also movedmaster ahead in the meantime so maybe you want to rebase or else we can simplify and squash-merge for simplicity (once you made the small editing changes suggested below).

Comments:

CHECK-PR on parameters: I'd say remove, in-line with what all other vignettes do. Not a
bad idea though. (We do conditional evaluation of chunks in the drat-data
paper in the R Journal, so the idea is fine in principle.)
So let's restore evaluation as before
All the typo fixes are excellent. Thank you so much!
Ditto with all the editing fixes. Very attentive.
CHECK-PR on resize: Likely name clash and good idea to work around. So we
should keep this and replace 'CHECK-PR' with something like 'FixForC++11'
CHECK-PR on mustStart=TRUE: Don't know
CHECK-PR on sourceCpp: Excellent. Absolutely keep. Nicely done.
CHECK-PR on chunk needed: That looks like a left-over style option from
JSS or something. Not needed I'd say.

riccardoporreca added7 commits

August 5, 2019 21:39

Harmonize inline examples, defer all package-related stuff.

f38135c

* As a preliminary step before reviewing the specific section about packges.* General refinement to make it easier for a user to follow and run the examples.* Now all inline examples assume fxx (clearly stated) and explicitly call Module(), to make the examples closer to self-contained.* Also fixed minor typos.

Complete World S4 dispatch example.

54cb17a

* World was not already included in the examples above.

Refine and fix inline module examples, also made runnable.

cdfa697

* Inline examples with R code now have a non-included chunk creating the assumed relevant fx object based on the C++ code in previous named chunk(s).* The document has gained a parameter `eval` in the YAML header, that can be set to TRUE to optionally run the examples.* This revealed compilation errors with the std::vector example, fixed with a workaround, to be discussed upon PR.

Major package section review; cover sourceCpp usage.

32b9327

* Focus on recommended usage via loadModule with example * General review, also covering exports.* New section about using the convenient sourceCpp.

Batch cosmetic refinements and alignment.

46eadae

* \proglang all the way.* \textsl{Rcpp modules} everywhere but in their own section.* Consistent usage of inline C++ elements.

Convenience wrapper to support evaluation +results chunk option.

5b6bbf4

* Make evaluation even less intrusive.

Include@eddelbuetteland@coatlessPRRcppCore#982feedback.

9832314

* Removed parameters for evaluating chunks, restored evaluation as before, keeping named Rcpp chunks.* Finalized comment about resize and push_back workarounds for C++11.* Referred to Rcpp attributes vignette for sourceCpp.* Removed mustStart=TRUE (undocumented), the example works without.* Removed left-over style option chunks not needed.