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

Testable search #505

Merged
merged 9 commits into from
Sep 9, 2024
Merged

Testable search #505

merged 9 commits into from
Sep 9, 2024

Conversation

vifactor
Copy link
Collaborator

@vifactor vifactor commented Jul 19, 2024

The search dialog core functionality is moved into a separate class.

Added unit tests for the class. Without this change I do not believe it is possible to make any "reliable" improvements for the search.
The unit tests are so far run only locally. CI adjustments can be done later if this PR is merged

The "payload range" functionality is not implemented: it does not work in the master-branch and requirements are unclear.

Since in this PR I use optional and variant I also switch on c++17 standard where these utils were introduced. If not acceptable, in principle code can be rewritten without them. See #501

I tested the refactored search functionality on several dlt file manually, so far I did not see issues.

@vifactor vifactor marked this pull request as draft July 19, 2024 19:52
@vifactor vifactor force-pushed the testable-search branch 3 times, most recently from 2a8b0a6 to 7e85ee4 Compare July 21, 2024 20:26
@alexmucde alexmucde added this to the Release v2.27.0 milestone Jul 23, 2024
If timestamp range search is selected
the loop will only continue if range is valid. It does not make sense
to check in functions below if range is valid since otherwise
that code cannot be even reached
@vifactor vifactor marked this pull request as ready for review July 23, 2024 20:03
move matcher to qdlt lib

enable c++17 in qmake config

The new code uses variant and optional
added in c++17 standard while qmake config was
setup restricted to c++11

enable tests only if gtest package found

Set c++17 standard in cmake

export class for windows linker

Signed-off-by: Viktor Kopp <[email protected]>
@vifactor
Copy link
Collaborator Author

vifactor commented Aug 8, 2024

@alexmucde any thoughts on this? in principle the PR is ready.
do you want me to rewrite it without using c++17? would you see it to be done differently? what about getting rid of the broken (and probably unused) payload range functionality?

@alexmucde
Copy link
Collaborator

Is c++1z the name for C++17?

@vifactor
Copy link
Collaborator Author

vifactor commented Aug 14, 2024

Is c++1z the name for C++17?

yes, this is the only way I was able to enforce it for "Build macOS (macos-13, Xcode_15.2)"-job. AFAIR, Win, Lin and Mac-14 were green if I use CONFIG += c++17 syntax. Here is the source I used: https://stackoverflow.com/a/51197053

If we the project maintained only cmake build system, things would be simpler and cleaner. But I guess there is a reason you have to maintain both.

@alexmucde alexmucde self-assigned this Sep 9, 2024
@alexmucde alexmucde merged commit c1a6854 into COVESA:master Sep 9, 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