Skip to content

Conversation

@firewave
Copy link
Collaborator

@firewave firewave commented Nov 29, 2023

The lastest release of picojson does not support creation of JSONs, so we need to switch to the current dev version.

@firewave
Copy link
Collaborator Author

The last picojson release was over 8 years ago and the last commit was almost 3 years ago so we can consider the library unmaintained and abandoned. In the future we should consider moving to a library that is still being supported. I filed https://trac.cppcheck.net/ticket/12233 about that.

It is still marked a draft since I have no tested this yet since the GUI is crashing on the system I am currently on - I filed https://trac.cppcheck.net/ticket/12229 about that.

@firewave firewave changed the title gui/mainwindows.cpp: use picojson to generate JSON / updated picojson to latest dev version gui/mainwindow.cpp: use picojson to generate JSON / updated picojson to latest dev version Nov 30, 2023
@firewave firewave marked this pull request as ready for review November 30, 2023 10:29
@danmar
Copy link
Owner

danmar commented Dec 1, 2023

The last picojson release was over 8 years ago and the last commit was almost 3 years ago so we can consider the library unmaintained and abandoned. In the future we should consider moving to a library that is still being supported. I filed https://trac.cppcheck.net/ticket/12233 about that.

good 👍

@danmar
Copy link
Owner

danmar commented Dec 1, 2023

If we only need to write json in the GUI then we could use the Qt JSON classes. If we need to drop support for Qt 5.x that is no biggie for me:

https://doc.qt.io/qt-6/json.html

@firewave
Copy link
Collaborator Author

firewave commented Dec 1, 2023

If we only need to write json in the GUI then we could use the Qt JSON classes. If we need to drop support for Qt 5.x that is no biggie for me:

https://doc.qt.io/qt-6/json.html

Thanks for noting. I would prefer though if we use the same library across the code.

I tried to use wrappers for the JSON stuff so it could be more easily replaced but we use it differently in most cases so it was quite awkward to do so. Maybe the pattern appears clearer after I have adjusted all usages.

@danmar
Copy link
Owner

danmar commented Dec 1, 2023

I would prefer though if we use the same library across the code.

Basically yes

however if we introduce a dev version there could be bugs.

I was thinking that maybe this was the only place where we need to write json. I don't spontanously know any other place. Then it might make sense to use the Qt library just here.

@firewave
Copy link
Collaborator Author

firewave commented Dec 1, 2023

however if we introduce a dev version there could be bugs.

Possible but our usage is so basic it is unlikely. It also introduced support for rvalue, noexcept etc. so it should allow for better code. Will profile again soon as part of some more usage cleanups.

I was thinking that maybe this was the only place where we need to write json. I don't spontanously know any other place. Then it might make sense to use the Qt library just here.

I think it is the only place so that would make sense - will take a look. We do it in the unit tests but there we just use a string as input.

@danmar
Copy link
Owner

danmar commented Dec 1, 2023

I think it is the only place so that would make sense - will take a look. We do it in the unit tests but there we just use a string as input.

I think that is enough for me.. if you take a serious look and still conclude that picojson dev is better I will trust your judgement..

@firewave
Copy link
Collaborator Author

firewave commented Dec 1, 2023

I think that is enough for me.. if you take a serious look and still conclude that picojson dev is better I will trust your judgement..

Not much was changed. Most of it is formatting/whitespaces and if you ignore those you see it is mostly some fixes and modernization. Looks pretty safe.

@danmar
Copy link
Owner

danmar commented Dec 5, 2023

@firewave alright I would say we can merge this

@danmar danmar merged commit 347b188 into danmar:main Dec 5, 2023
@firewave firewave deleted the picojson-set branch December 5, 2023 18:03
firewave added a commit that referenced this pull request Dec 5, 2023
The tests which are failing were in introduced in #5712. Those were not
included in #5710 which updated `picojson` that resulted in the
different lines being reported in the error messages.

I also added some checks to `ScopeFile` which will indicate that a
temporary file already exists highlighting multi-threading issues and
leftover files from previously aborted testruns. I ran into his while
looking into these failing tests
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.

2 participants