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

Upgrade RapidJSON #4051

Merged
merged 2 commits into from
Mar 30, 2023
Merged

Upgrade RapidJSON #4051

merged 2 commits into from
Mar 30, 2023

Conversation

ianthetechie
Copy link
Contributor

@ianthetechie ianthetechie commented Mar 30, 2023

Issue

Fixes #4050.

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

@ianthetechie
Copy link
Contributor Author

This does not currently add any new tests, but it does fix UB in an existing test. We are currently permissive in our sanitized runs and do not throw an error.

I would suggest building with -fno-sanitize-recover eventually as the "test case" after #4049 is addressed.

@nilsnolde
Copy link
Member

nilsnolde commented Mar 30, 2023

Always a tough decision when a project isn't releasing anymore.. 7 years and counting.. Though the project is far from being dead apparently. @mloskot summarizes it well 😄

Since there's maintenance going on, I'd personally be in favor of upgrading to the latest commit, seeing how we're vendoring it anyways (plus our own "test coverage" for rapidjson is quite high I'd say). Curiously they only seem to test windows though, never seen that on a C++ repo haha

Copy link
Collaborator

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

Apart from the generic "ain't broke, don't touch", I don't see any reason to not to upgrade

@kevinkreiser
Copy link
Member

yeah i want to echo that sentiment from @mloskot

i feel like a lot of times people make too much of a big deal out of "unmaintained". the json spec itself reached its latest revision 5 years ago but has existed for like 20 years more or less. rapidjson, and many other projects like it, are small convenience libraries that do one job well. barring any bug fixes, what on earth else would "maintained" even signify? in summary, the library implements a spec which is mature for a long time and is narrowly focused on that point, i see no reason to worry at all about it being "unmaintained". ship it!

@kevinkreiser kevinkreiser merged commit e3aa7f6 into valhalla:master Mar 30, 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.

UB in RapidJSON
4 participants