Skip to content

Conversation

eddelbuettel
Copy link
Contributor

As discussed in #2343, this aims to avoid a freshly added nag from clang++-20 about deprecated whitespace in operator literal declarations.

@lemire
Copy link
Member

lemire commented Mar 6, 2025

@eddelbuettel I am skeptical of this PR because you are patching a secret dependency that we have... and this dependency should only ever come in if std::string_view is missing. Yet you are making a reference in the issue to C++20... but C++20 requires std::string_view. So it seems likely that this PR does no good. (It is also probably not harmful per se.)

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Mar 6, 2025

🤷‍♂️

I have you the link to Brian Ripley's tests on Fedora with clang++-20. I have no more details. You the know R package will not pull anything else in. And that ends it. [ Also, to be plain, the only '20' I mentioned is the compiler version. Not the C++ standard. The fuller log shows it builds as C++17 there at CRAN. ]

Also, I have zero choice in the matter. If I don't change RcppSimdJson, they will throw it off CRAN. So I will change it. Now if you don't (as I fully know there may be reasons) then we have a delta between upstream and downstream which nobody likes. So mull it over, but if in doubt consider merging. I do not see how the PR, minimal and white-space focussed as it is, can do harm. But hey what do I know.

(Impressive battery of tests. I had to click 'cancel' for minutes on end at my GH repo of simdjson. I disabled actions there for now.)

@eddelbuettel
Copy link
Contributor Author

Turns out I overlooked that the issue at CRAN had already been fixed, see #2343 (comment). So closing this and sorry for the noise. It was the third such fix in 24 hours (after date and toml++ and I had gotten sloppy.

@striezel
Copy link
Contributor

@eddelbuettel I am skeptical of this PR because you are patching a secret dependency that we have...

Yes, I agree. The way this was handled in commit f3b034a is wrong, because it only fixes the symptoms here and does not address the root cause.

The proper way to address an issue in any dependency would be to open a PR to fix it in the upstream repository, and that's what I did: nonstd-lite/string-view-lite#61 Now others using string-view lite can benefit from that change/fix, too.

@eddelbuettel
Copy link
Contributor Author

Sure. I 'merely' failed to recurse as simdjson is one level up from RcppSimdJson where we were bitten. It's not an excude but this was fairly widespread among the approximately 22k CRAN packages of which 5k (give or take) have compiled code and hit a good few dozen among those. I think I filed three PRs, but I am 2 for 3 as it wasn't right here, which I could have seen when I filed it as even the 'symptom' I was fighting here had already been fixed. And then I got carried away looking in other places. And that was the other issue. Lesson learned.

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