Auto generation Warnings & Invalid C++ Identifiers (Closes #526 and #387) #528

Merged
merged 7 commits into from Aug 3, 2016

Projects

None yet

3 participants

@coatless
Contributor
coatless commented Aug 2, 2016 edited

Added Autogeneration Warnings & Invalid C++ Identifiers (Closes #526 and #387)

@coatless coatless changed the title from Autogeneration warning to Auto generation Warnings & Invalid C++ Identifiers (Closes #526 and #387) Aug 2, 2016
@jjallaire jjallaire commented on an outdated diff Aug 2, 2016
src/attributes.cpp
<< std::endl;
if (!function.type().isVoid()) {
ostr() << " return Rcpp::as<" << function.type() << " >"
- << "(__result);" << std::endl;
+ << "(RCPP_result_gen_);" << std::endl;
@jjallaire
jjallaire Aug 2, 2016 Member

I'd say drop the trailing underscore as this convention is used elsewhere (e.g. in the attributes implementation) to indicate a C++ member variable.

@jjallaire jjallaire and 1 other commented on an outdated diff Aug 2, 2016
src/attributes.cpp
@@ -2196,7 +2197,7 @@ namespace attributes {
}
std::string CppPackageIncludeGenerator::getHeaderGuard() const {
- return "__" + packageCpp() + "_h__";
+ return "RCPP_" + packageCpp() + "_h_gen_";
@jjallaire
jjallaire Aug 2, 2016 Member

Here I think we want to add a leading underscore (and keep the trailing underscore) to be consistent with the use of header guards elsewhere.

@coatless
coatless Aug 2, 2016 edited Contributor

To confirm, does this mean:

"_rcpp_" + packageCpp() + "_h_gen_"

or

"_RCPP_" + packageCpp() + "_h_gen_"?

@jjallaire
jjallaire Aug 2, 2016 Member

@eddelbuettel had suggested capitals (which is generally what I've seen in header guards as well). I don't have a super strong preference but I think we should be consistent, e.g. either:

"_rcpp_" + packageCpp() + "_h_gen_"

or

"_RCPP_" + packageCpp() + "_H_GEN_"
@eddelbuettel
Member

We now have a merge conflict. Do you want to / have time to work through it? Else I can merge it manually later bu then you don't get the glory ...

@eddelbuettel
Member

Will merge this as nobody cried foul ...

@eddelbuettel eddelbuettel merged commit d18a0bc into RcppCore:master Aug 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ghost
ghost commented Sep 2, 2016 edited

Hi, might I suggest to drop the leading underscore at all? So instead of writing:

    return "_RCPP_" + packageCpp() + "_RCPPEXPORTS_H_GEN_";
    ...
    return "_RCPP_" + packageCpp() + "_H_GEN_";

just write:

  return "RCPP_" + packageCpp() + "_RCPPEXPORTS_H_GEN_";
  ...
  return "RCPP_" + packageCpp() + "_H_GEN_";

See our discussion on stackoverflow: http://stackoverflow.com/questions/39291133/how-to-change-header-include-guards-in-rcpp-interfaces/39291326#39291326 and http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier/228797#228797

All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

Best,
Simon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment