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 export rules header #608

Merged
merged 6 commits into from
Jan 7, 2025
Merged

Fix export rules header #608

merged 6 commits into from
Jan 7, 2025

Conversation

vifactor
Copy link
Collaborator

@vifactor vifactor commented Jan 4, 2025

The first commit is the most important, it fixes includes that I missed here: #594

Remove a bit of dead code in follow up commits. Can be reviewed commit-by-commit

This one is truly independent of QtGlobal header
Previous attempt to avoid C++ header in C code appeared to
be incomplete. The header QtGlobal  was removed, but
macros defined in that header remained in export_rules.h
The compilation did not fail until now most probably because
by accident all C++ file including export_rules.h also explicitely
or implicitly include QtGlobal.

Signed-off-by: Viktor Kopp <[email protected]>
Remove dead code

Signed-off-by: Viktor Kopp <[email protected]>
Cleanup dltargument

Signed-off-by: Viktor Kopp <[email protected]>
Remove unnecessary code

Signed-off-by: Viktor Kopp <[email protected]>
@vifactor vifactor added the merge label Jan 5, 2025
@vifactor vifactor marked this pull request as ready for review January 5, 2025 11:09
@vifactor vifactor added this to the Release v2.28.0 milestone Jan 5, 2025
@vifactor vifactor requested a review from alexmucde January 5, 2025 11:12
@alexmucde alexmucde self-assigned this Jan 7, 2025
@alexmucde alexmucde merged commit 5a66b24 into master Jan 7, 2025
6 checks passed
@vifactor vifactor deleted the export-rules branch January 7, 2025 21:04
@alexmucde
Copy link
Collaborator

@vifactor I have integrated the plugin interface changes, but know i have a lot of issues in our internal plugins, similar to what you have changed already in the OSS plugins. Now i have two options. Revert your changes or walk through each issue and fix it.

@vifactor
Copy link
Collaborator Author

vifactor commented Jan 9, 2025

@alexmucde you mean you need to do this type of changes in your private plugins, right?
image

If so, give me a couple of hours to think how this can be fixed with minimal nb of changes, because I find changes made in this MR are good. I understand the pain of fixing multiple places, so if you think it is easier, feel free to revert of course, and sorry for that, I did not expect I would break API

@alexmucde
Copy link
Collaborator

I see you changes are useful, but will break the interface. I will first give it a try to fix it in our plugins. If it works easily we can keep your changes. I hope a find and replace will help.

@vifactor
Copy link
Collaborator Author

vifactor commented Jan 9, 2025

okay, so the easiest way to fix your plugins IMO and not revert these changes is to simply define:

class QDltMsg {
  [...]
  static QString toAsciiTable(...) {
      return QDlt::toAsciiTable(...);
  }
  
  [...]
}

I think this should do the job

P.S. For the Endianness enum scope, I'm not sure how to handle it in a clean way, unfortunately

P.S.2. Dirty way to fix things is just again inherit from QDlt in the QDltMsg class

@alexmucde
Copy link
Collaborator

alexmucde commented Jan 9, 2025

The first things were easily fixed. But you also have removed some arrays, which i have used in a plugin:
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1110: Fehler: Use of undeclared identifier 'qDltMessageType'
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1115: Fehler: Use of undeclared identifier 'qDltLogInfo'; did you mean 'QDltMsg::DltLogInfo'? (fix available)
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1115: Fehler: Subscripted value is not an array, pointer, or vector
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1118: Fehler: Use of undeclared identifier 'qDltTraceType'
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1121: Fehler: Use of undeclared identifier 'qDltNwTraceType'
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1124: Fehler: Use of undeclared identifier 'qDltControlType'; did you mean 'QDltMsg::DltControlTime'? (fix available)
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1124: Fehler: Subscripted value is not an array, pointer, or vector
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1183: Fehler: Use of undeclared identifier 'qDltTypeInfo'
C:\workspace\github\covesa\dlt-viewer_plugin_development\plugin\adnonverboseplugin\adnonverboseplugin.cpp:1190: Fehler: Use of undeclared identifier 'qDltTypeInfo'

@vifactor
Copy link
Collaborator Author

vifactor commented Jan 9, 2025

oh, I could not imagine those vars were part of the API. They moved to cpp files, fix should be also easy: just move them back to QDlt-header, but keep inline keyword

@alexmucde
Copy link
Collaborator

I have fixed it now by adding the vars to the plugin as you have done.

The following line still makes some issues qdltbase.h:
inline constexpr const auto DLT_MAX_MESSAGE_LEN = 1024*64;

When i remove "inline" it works, looks like it is incompatible with older C++ standards and our plugins are build with older C++ standard. I will create a PR for this.

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