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

fix to the issue #90 #91

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

fix to the issue #90 #91

wants to merge 9 commits into from

Conversation

ekamioka
Copy link

@ekamioka ekamioka commented Aug 29, 2018

Hey @Ironholds

AlexCP said maybe I can help, so here is my PR in hope to solve it.

I added new tests to ensure I am doing the thing (and also not breaking the codebase). Running results was:

==> Testing R file using 'testthat'

✔ | OK F W S | Context
✔ | 55       | URL parsing tests [0.2 s]

══ Results ════════════════════
Duration: 0.3 s

OK:       55
Failed:   0
Warnings: 0
Skipped:  0

Test complete

@Ironholds
Copy link
Owner

Does this work nicely on Windows machines as well? I know there are historical legacy issues with getting R to play nice with the regex header there.

src/parsing.cpp Outdated
auth = url.find("@");
}
if(auth != std::string::npos){
std::regex rx ("^(\\w+:\\w+@).*");
Copy link
Owner

Choose a reason for hiding this comment

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

Might suggest compiling the regex on package instantiation for a speed bump rather than recompiling it with every individual call? This is going to mean a lot of compilations!

@alexcpsec
Copy link
Collaborator

@Ironholds I know nothing about RCpp packages, but can we add the --std=c++11 flag to the build process so it can add that independently if I do not have it on my RMakevars ?

@Ironholds
Copy link
Owner

Hm; I don't remember either. I think the DESCRIPTION should now let you specify? It's been a while.

@ghost
Copy link

ghost commented Dec 17, 2018

@Ironholds I know nothing about RCpp packages, but can we add the --std=c++11 flag to the build process so it can add that independently if I do not have it on my RMakevars ?

I've just tested and adding PKG_CXXFLAGS=-std=c++11 to .R/Makevars or setting Sys.setenv("PKG_CXXFLAGS"="-std=c++11") will do the work.

PS. gcc version must be greater or equals to 4.9

@Ironholds
Copy link
Owner

And does R now require C++11 support? (that is, is it likely to be on target machines?)

@alexcpsec
Copy link
Collaborator

alexcpsec commented Dec 18, 2018 via email

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.

3 participants