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

Sync with Upstream #11

Closed
knapply opened this issue May 20, 2020 · 10 comments
Closed

Sync with Upstream #11

knapply opened this issue May 20, 2020 · 10 comments

Comments

@knapply
Copy link
Collaborator

knapply commented May 20, 2020

Per the contributing guidance, here is a planned PR.

The main motivation (beyond keep pace) is access to simdjson::dom::object and ::array sizes for better preallocation.

I'm happy to update NEWS.Rd if the following conforms...

...

\section{Changes in version 0.0.5 (2020-xx-xx)}{
  \itemize{
    \item Synced with upstream (Brendan in \ghpr{x}
    \item Updated example \code{parseExample} to API changes.
  }
}

...
@eddelbuettel
Copy link
Owner

That's prize winning material :) Appreciate the attention to detail. Following the initial few releases which ironed out some things (though eg the stderr reverted as your noticed) updates should be "cheap". Because we don't have much in terms of usage there isn't much in terms of test coverage either.

@knapply
Copy link
Collaborator Author

knapply commented May 20, 2020

Beyond being insanely fast, the simdjson API is really slick. More functionality, tests, and users seem inevitable.

I wanted to sync before tackling anything else (including giving @dcooley a second set of eyes for from_json()), but I definitely have some ideas that I've been playing with elsewhere. I'll bring those up separately when time permits.

@eddelbuettel
Copy link
Owner

eddelbuettel commented May 20, 2020

One thing to keep in mind is that they code and change things almost as fast as the code runs :) Kidding aside on my last upgrade early April I had to change maybe four instances even though there are only so few lines of code (yet). But as usual we can expect the API to stabilize as it matures. And the changes were all great.

@lemire
Copy link
Collaborator

lemire commented May 20, 2020

One thing to keep in mind is that they code and change things almost as fast

I understand what you mean, but for the folks reading this who are not familiar with the matter, let me say: that is slightly unfair. We introduced a new (better?) API while keeping the old one. People who coded against our releases probably did not have any broken code.

The new API may change a bit, maybe... but it is unlikely to change a lot. At best, we might introduce a few things and deprecate others but we are simply not going to willingly break anyone's code, and we will surely not do it on a whim.

We code very, very carefully and rather slowly.

We hate having APIs change from under us, so we don't do the same to others.

cc @jkeiser

@eddelbuettel
Copy link
Owner

It was meant as a self-deprecating joke, it didn't work. My bad.

I was referring to the last part of this commit which shows that a function barely four lines long required changes on four lines.

I am not implying you are not responsible about API stability. I was merely joking that I was glad that day that I had four lines. Not four hundred. The code is better now, and change absolutely can be worth it. E.g. I dig auto [doc, error] = parser.load(filename.c_str()); which was a first for me (often being constrained by there may be a red-haired orphan in a valley still using a centos version with a compiler from the 18th century yada yada).

Again, if I stepped on toes, my bad.

@lemire
Copy link
Collaborator

lemire commented May 20, 2020

It was meant as a self-deprecating joke, it didn't work. My bad.

Yes, yes, I understood... please see the "I understand what you mean, but for the folks reading this " part of my comment!!! I was merely concerned with your comment being taken out of context.

@knapply knapply closed this as completed May 21, 2020
@jkeiser
Copy link

jkeiser commented May 21, 2020

Yeah, it's all good. And yeah, we're pursuing a policy of deprecation to give people time to change, and even for the things we deprecate, we try to say "OK, the user is going to be annoyed at having to change their code. Will they feel like the API change itself is buying them some great new capability?" If the answer is no, then we don't want to change the API, even if it feels nice from a refactoring point of view or something.

@eddelbuettel
Copy link
Owner

eddelbuettel commented May 22, 2020

So ... having the update from #12 in here, I contemplated uploading to CRAN. The build service at https://builder.r-hub.io has a convenient check_for_cran() portmonteau caller which I sometimes use. It sends to windows (passed, we now use gcc-8), ubuntu 16.04 (dies, can't do g++-17), fedora with r-devel and clang (passes) and debian with r-devel with r-devel and asan -- with groans about deprecation:

RcppSimdJson 0.0.4.1: ERROR

Build ID:   RcppSimdJson_0.0.4.1.tar.gz-77968326a4094816a0c4afadae992370
Platform:   Debian Linux, R-devel, GCC ASAN/UBSAN
Submitted:  5 minutes 17.5 seconds ago
Build time: 5 minutes 16.8 seconds

ERRORS:
-------
*
About to run xvfb-run san.sh RcppSimdJson_0.0.4.1.tar.gz
* installing to library ‘/home/docker/R’
* installing *source* package ‘RcppSimdJson’ ...
** setting up C++17
** libs
g++ -fsanitize=undefined,bounds-strict -fno-omit-frame-pointer -std=gnu++17 -I"/usr/local/lib/R/include" -DNDEBUG  -I"/home/docker/R/Rcpp/include" -I/usr/local/include  -DSIMDJSON_NO_COMPUTED_GOTO
+-I../inst/include  -fpic  -g -O2 -Wall -pedantic -mtune=native -c RcppExports.cpp -o RcppExports.o
g++ -fsanitize=undefined,bounds-strict -fno-omit-frame-pointer -std=gnu++17 -I"/usr/local/lib/R/include" -DNDEBUG  -I"/home/docker/R/Rcpp/include" -I/usr/local/include  -DSIMDJSON_NO_COMPUTED_GOTO
+-I../inst/include  -fpic  -g -O2 -Wall -pedantic -mtune=native -c simdjson_example.cpp -o simdjson_example.o
In file included from simdjson_example.cpp:6:0:
../inst/include/simdjson.h:4433:38: warning: ‘Iterator’ is deprecated [-Wdeprecated-declarations]
   inline Iterator(const Iterator &o) noexcept;
                                      ^~~~~~~~
../inst/include/simdjson.h:4436:49: warning: ‘Iterator’ is deprecated: Use the new DOM navigation API instead (see doc/basics.md) [-Wdeprecated-declarations]
   inline Iterator& operator=(const Iterator&) = delete;
                                                 ^~~~~~
../inst/include/simdjson.h:4430:97: note: declared here
 class [[deprecated("Use the new DOM navigation API instead (see doc/basics.md)")]] dom::parser::Iterator {
                                                                                                 ^~~~~~~~
../inst/include/simdjson.h:4436:49: warning: ‘Iterator’ is deprecated [-Wdeprecated-declarations]
   inline Iterator& operator=(const Iterator&) = delete;
                                                 ^~~~~~
simdjson_example.cpp: In function ‘bool validateJSON(std::__cxx11::string)’:
simdjson_example.cpp:14:19: warning: unused variable ‘doc’ [-Wunused-variable]
   auto [doc, error] = parser.load(filename.c_str()); // do the parsing
                   ^
g++ -fsanitize=undefined,bounds-strict -fno-omit-frame-pointer -std=gnu++17 -shared -L/usr/local/lib/R/lib -L/usr/local/lib -o RcppSimdJson.so RcppExports.o simdjson_example.o -L/usr/local/lib/R/lib -lR
installing to /home/docker/R/RcppSimdJson/libs

Guess I have some reading to do in doc/basics.md

Edit: Or maybe not. My box (g++-9.2) is silent with/without -Wdeprecated-declarations. To be continued...

@lemire
Copy link
Collaborator

lemire commented May 22, 2020

ubuntu 16.04 (dies, can't do g++-17)

We support C++11. I think ubuntu 16.04 has a GNU GCC version that predates GNU GCC 7. Hmmm GNU GCC 4.7 I think. That's very, very old.

with groans about deprecation: (...)

I don't think you need to worry too much about that. We have a new API which is much nicer but it does imply a rewrite of sort. We still support the old API and there is no plan to retire it, but we do encourage you to update your code to the new API.

After initially swearing a bit, I am sure you will agree that it is much nicer once you grok it.

@eddelbuettel
Copy link
Owner

Also the test that barked was the ASAN one which simulatenously helpful (the *SAN stuff is amazing) but also known to toss out false positives.

So I think I will wrap up what I have (thanks again @knapply !) and 'ship it' to CRAN.

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

No branches or pull requests

4 participants