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

Quoting only non-standard paths on linux #1243

Merged
merged 2 commits into from Jan 24, 2023

Conversation

llaniewski
Copy link
Contributor

This closes #1242

Checklist

  • 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

Copy link
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.

Thanks for adding changelog and news, and flipping from = to <-. I would probably still reorder the logic in function to (untested)

Rcpp.quoteNonStandard <- function(path, quoteAll = .Platform$OS.type!="unix") {
    ## Check if path has only characters that do not need quoting
    sel <- grepl("^[[:alnum:]/._~+@%-]*$", path)
    ## If no quoting needed return unchanged else quote input
    if (sel) path else shQuote(path)
}

Simpler?

@llaniewski
Copy link
Contributor Author

I simplified the function for single path input, and removed the option, as it was not used. Hope this looks good now.

Copy link
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.

👍 -- looks great! I'll wait a bit in case Kevin or Inaki find something objectionable but will otherwise merge and roll up a first 1.0.10.1 with this.

@Enchufa2
Copy link
Member

For the regexp, I suggest following some well-established reference. For example, this is what pkgconfig escapes in paths:

if ((s[0] < '+') ||
    (s[0] > '9' && s[0] < '@') ||
    (s[0] > 'Z' && s[0] < '^') ||
    (s[0] == '`') ||
    (s[0] > 'z')) {
	r[c] = '\\';
	c++;
}

So the inverse would be allowed characters.

@eddelbuettel
Copy link
Member

Good idea in theory. In practice ... maybe I am lacking coffee (only one cup in) but what exactly are you proposing? A per-character loop? A re-written regexp? Can you sketch something I actually think the regexp is reasonably careful as is.

@Enchufa2
Copy link
Member

Enchufa2 commented Jan 24, 2023

A re-written regexp. Following the ASCII table and the reference above, it seems after all that the only difference would be ^ instead of %. But I don't see why % would require quotation. :-\ I suppose it's good enough as it is. Don't mind me. Sorry for the noise.

@eddelbuettel
Copy link
Member

Extra eyes on the ball are a good thing. Thanks for checking!

@eddelbuettel eddelbuettel merged commit d9d551d into RcppCore:master Jan 24, 2023
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.

Path quoting in CxxFlags with bash
3 participants