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

Fix more compilation warnings #499

Merged
merged 8 commits into from
Jul 23, 2024
Merged

Conversation

vifactor
Copy link
Contributor

Mainly due to deprecated methods and integer comparison of different signedness.

@vifactor
Copy link
Contributor Author

vifactor commented Jul 11, 2024

I used one c++17 feature but then noticed that project is set to built with c++11. Is this a strong requirement?

Edit: it seems important, windows CI fails. Fixed

@alexmucde
Copy link
Collaborator

@vifactor Thanks for your contribution. Is it possible to add a signed off statement with your name and eMail address and in your last or a new commit message?

@vifactor
Copy link
Contributor Author

@alexmucde sure, done

@alexmucde
Copy link
Collaborator

@vifactor Sorry i was working on a parallel task. Can you please rebase to master? The file src/dltimporter.cpp is now moved to qdlt/qdltimporter.cpp

due to comaprison of ints with different signedness
comparison of integer expressions of different signedness
comparison of integer expressions of different signedness
 using integer constants in boolean context
Signed-off-by: Viktor Kopp <vifactor@gmail.com>
@vifactor
Copy link
Contributor Author

@alexmucde rebased

@vifactor
Copy link
Contributor Author

@alexmucde do I need to do anything else to make this merged?

@alexmucde
Copy link
Collaborator

@vifactor Should be fine now, but i would like to test it before integration. Have you already tested the affected parts of the SW, e.g. MF4 import?

@vifactor
Copy link
Contributor Author

@alexmucde I thought my changes were relatively trivial. But, of course, you are right better to verify even trivial changes. Unfortunately, I do not think I have MF4 files. Do you have some samples?

@vifactor
Copy link
Contributor Author

@alexmucde I found some mf4 files here: https://canlogger.csselectronics.com/canedge-getting-started/ce2/_downloads/de00db1faa001b51fce93666ebaba20e/mf4-sample-data-v2.1.zip

I tried dlt-viewer v2.21.2 and the version from master. In both cases when I import files, no logs are seen in the viewer UI, however console logs show that something gets imported. Maybe I simply do not know how to check that functionality. Would appreciate some help

@vifactor
Copy link
Contributor Author

@alexmucde would this PR have higher chances to be merged sooner if I move so far untested changes for qdlt/qdltimporter.cpp into a separate PR?

@alexmucde
Copy link
Collaborator

@vifactor I have tested MF4 import and it is still working. I will merge, but further testing needed.

@alexmucde alexmucde merged commit 7024a1a into COVESA:master Jul 23, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants