Skip to content

use ctest and set path to dbc files with a compile definition #9

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

Merged
merged 14 commits into from
Jan 12, 2023

Conversation

Murmele
Copy link
Contributor

@Murmele Murmele commented Jan 6, 2023

No description provided.

@Murmele Murmele mentioned this pull request Jan 7, 2023
Copy link
Owner

@LinuxDevon LinuxDevon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when you are ready for review on this one. Here are some thoughts I had looking over it. Thank you for the work on this one!

Comment on lines 59 to 70

auto* file = std::fopen(filename, "w");
CHECK(file);

std::fputs(PRIMITIVE_DBC.c_str(), file);
std::fputs(R"(BO_ 234 MSG1: 8 Vector__XXX
SG_ Sig1 : 55|16@0- (0.1,0) [-3276.8|-3276.7] "C" Vector__XXX
SG_ Sig2 : 39|16@0- (0.1,0) [-3276.8|-3276.7] "C" Vector__XXX
SG_ Sig3 : 23|16@0- (10,0) [-3276.8|-3276.7] "C" Vector__XXX
SG_ Sig4 : 7|16@0- (1,-10) [0|32767] "" Vector__XXX)", file);
std::fclose(file);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quick refactor I would suggest is to pull this out into a function void create_tmp_dbc_with(const std::string_view content)at the top of the test file here. That way you can pull out the duplicate code in the case below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would preferr not doing this, because consider you are adding additional tests then you have to create another function. I also don't like copy paste, but in tests I prefer, so when I change it at one position I don't break all tests.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess why would you need another function in that case? The only differences are the content you are putting in the tmp file right so the two function calls would like such:

create_tmp_dbc_with(R"(BO_ 234 MSG1: 8 Vector__XXX
 SG_ Sig1 : 55|16@0- (0.1,0) [-3276.8|-3276.7] "C" Vector__XXX
 SG_ Sig2 : 39|16@0- (0.1,0) [-3276.8|-3276.7] "C" Vector__XXX
 SG_ Sig3 : 23|16@0- (10,0) [-3276.8|-3276.7] "C" Vector__XXX
 SG_ Sig4 : 7|16@0- (1,-10) [0|32767] "" Vector__XXX)"
);

// In the other test case
create_tmp_dbc_with(R"(BO_ 234 MSG1: 8 Vector__XXX
 SG_ Speed : 0|8@1+ (1,0) [0|204] "Km/h"  DEVICE1,DEVICE2,DEVICE3)"
);

Copy link
Owner

@LinuxDevon LinuxDevon Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual function would be:

void create_tmp_dbc_with(const std::string_view content)
{
  auto* file = std::fopen(filename, "w");
  CHECK(file);

  std::fputs(PRIMITIVE_DBC.c_str(), file);
  std::fputs(content, file);
  std::fclose(file);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok I understand yes this would be possible

Copy link
Contributor Author

@Murmele Murmele Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine using const char* instead of string_view, because string_view requires c++17 and labplot currently uses c++11.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed in a666b52

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea that is fine with just const char *. I forgot that i had it set to c++11. I left a comment about the inclusion of the <string_view> header. Once that is removed I am good to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it in 2a4d059

@LinuxDevon LinuxDevon merged commit 1cfa210 into LinuxDevon:master Jan 12, 2023
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