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

C++ 20 #138

Open
sim186 opened this issue Oct 15, 2024 · 12 comments
Open

C++ 20 #138

sim186 opened this issue Oct 15, 2024 · 12 comments
Assignees

Comments

@sim186
Copy link

sim186 commented Oct 15, 2024

Hi Jan,

I was trying to switch my project to C++20 standard, do you know if there are issues using the standard with the library? I am getting a crash but I am not sure if the problem is on my side. Looks like that something has changes with the management of the un-initialized variables.

Do you have an idea?
Thanks

@jkriege2
Copy link
Owner

In principle it should work (and I think it worked, at least half a year ago) ... I added a C++20-build to the Appveyor-build ... and will test it again locally.

BTW: You can set the CMake-option "JKQtPlotter_ENABLED_CXX20=ON", which should activate some C++20-features in the code (e.g. use of std::format())... and it will require at least C++20 as a c++-standard-version for the CMake-build.

I'll let you know what further tests transpire ...

@jkriege2 jkriege2 self-assigned this Oct 15, 2024
@jkriege2
Copy link
Owner

locally on my desktop (Windows) I was able to build JKQTPlotter using C++20 with either MinGW64 or MSVC64 against Qt 6.7.2. Also some of the examples were working ...

@jkriege2
Copy link
Owner

What kind of error do you get?

@Snolandia
Copy link
Contributor

Running c++20 on my qt build. Builds and starts fine, though I do get a random crash presumable due to an uninitialized variable somewhere. I had changed the project from a previous version to 20 not too long ago, but its not clear if the crash was related to the version change or not.

Its not clear to me whether my crash is relevant to the Master Branch or my custom setup. I have not been able to reliably cause the crash so determining the cause has not been fruitful. To make it worse, the crash will not happen if I am running in debug mode, though the crash does happen with the debug build, while not in debug mode.

Are you able to reliably get it to crash? And at what point does it crash?

@jkriege2
Copy link
Owner

No, for me it works in the tests I ran ...

@Snolandia
Copy link
Contributor

No, for me it works in the tests I ran ...

I meant sim186.

@sim186
Copy link
Author

sim186 commented Oct 16, 2024

What kind of error do you get?

Hi. I get the crash in the addGraph call. The funny this as @Snolandia said in release configuration is working fine. As soon I get to the problem I try to give you more information.

@Snolandia
Copy link
Contributor

What kind of error do you get?

Hi. I get the crash in the addGraph call. The funny this as @Snolandia said in release configuration is working fine. As soon I get to the problem I try to give you more information.

No, I havent tried it in release configuration.

Debug config, building and running without it actually being in debug mode is where i get the crash.
Debug config, building and running with it being in debug mode, I cant get the crash to happen.

You say you get the crash in addGraph call, do you have anything initialized as a nullptr? Ive run into crashes when I initialize certain things as nullptr's and Qt does not like it.

@jkriege2
Copy link
Owner

I played around a bit with adress-sanitizer (in MSVC), but couldn't find any issues with several of the examples ... neither with C++20, nor with C++17 ...

@sim186
Copy link
Author

sim186 commented Oct 22, 2024

After digging for some days this is what I have found.

Compiling the library with C++17, some format stuff is excluded (see member “QString tickFormatFormat”) so the instance size (e.g. JKQTPCoordinateAxisStyle) has reduced size (472 Bytes).

Using the same instance from a C++20 compiled application, the size of JKQTPCoordinateAxisStyle changes to 496 bytes (one additional QString (+24 bytes). But the data was initialized for the smaller size, so the member are wrong initialized and there might be access outside of the 472 bytes data (access vialoation).
JKQTPlotter → plotter(JKQTBasePlotter) → plotterStyle → xAxisStyle -> tickFormatFormat (QString) is only available in C++20 → sizechange of QString (24 bytes) size change!

The same for yAxisStyle (+24 bytes) and on some other position again 2x 24 bytes → 96 bytes change in total.

JKQTBasePlotter: 6344 bytes (C++17) vs. 6440 bytes (C++20) → diff 96 bytes (see member plotterStyle next line)

JKQTBasePlotterStyle: 5032 bytes (C++17) vs. 5128 bytes (C++20) → diff 96 bytes (has members AxisStyle, see next line)

JKQTPCoordinateAxisStyle: 472 bytes (C++17) vs. 496 bytes (C++20) → diff 24 bytes (multiple times)

Senza titolo
Senza titolo-1

The classes should have the same layout for different C++ standards/configuration. Otherwise every user must ensure that the thirdy-party library is compiled with the same standard/configuration used (that can be bug-prone expecially with vcpkg).

I hope that this is helpful for you in order to solve the issue.
Thank you very much for your effort for this great library and the fast response.

@jkriege2
Copy link
Owner

Interesting ... and makes sense somehow ...

I'll think a bit about this, but in the end, it should be sufficient to always have the data member for the std::format format in the class ... just not using it.

What do you think?

Maybe I'll change the code accordingly later this week ... then we can test!

@jkriege2
Copy link
Owner

PS: and thanks for all the effort you put onto this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants