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.

Sign up for GitHub

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

Use local variable in btree2 and print value #4679

Merged
merged 4 commits into from
Aug 9, 2024
Merged

Use local variable in btree2 and print value #4679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
962694b
Use local variable in btree2 and print value
byrnHDF Jul 30, 2024
d57607b
Committing clang-format changes
github-actions[bot] Jul 30, 2024
4250597
Correct logic
byrnHDF Jul 30, 2024
ffa9e64
Technically, level 1 Express could skip tests
byrnHDF Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions test/btree2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9917,19 +9917,21 @@ main(void)
unsigned reopen; /* Whether to reopen B-tree during tests */
const char *driver_name;
bool api_ctx_pushed = false; /* Whether API context pushed */
int localTestExpress; /* localized TestExpress */

driver_name = h5_get_test_driver_name();

/* Reset library */
h5_test_init();
fapl = h5_fileaccess();
fapl = h5_fileaccess();
localTestExpress = TestExpress;

/* For the Direct I/O driver, skip intensive tests due to poor performance */
if (!strcmp(driver_name, "direct"))
SetTestExpress(2);
if (localTestExpress < 2 && !strcmp(driver_name, "direct"))
localTestExpress = 2;

if (TestExpress > 1)
printf("***Express test mode on. Some tests may be skipped\n");
if (localTestExpress > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why the change from 1 to 0 here? All the other places in this PR use 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, level one could skip tests. The other uses actually skip tests if level 2 or 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, level one could skip tests. The other uses actually skip tests if level 2 or 3.

Fixed other files.

printf("***Express test mode %d. Some tests may be skipped\n", localTestExpress);

/* Initialize v2 B-tree creation parameters */
init_cparam(&cparam, &cparam2);
Expand Down Expand Up @@ -9965,7 +9967,7 @@ main(void)
nerrors += test_insert_level2_2internal_split(fapl, &cparam, &tparam);
nerrors += test_insert_level2_3internal_redistrib(fapl, &cparam, &tparam);
nerrors += test_insert_level2_3internal_split(fapl, &cparam, &tparam);
if (TestExpress > 1)
if (localTestExpress > 1)
printf("***Express test mode on. test_insert_lots skipped\n");
else
nerrors += test_insert_lots(fapl, &cparam, &tparam);
Expand All @@ -9979,7 +9981,7 @@ main(void)
nerrors += test_update_level1_3leaf_redistrib(fapl, &cparam2, &tparam);
nerrors += test_update_level1_middle_split(fapl, &cparam2, &tparam);
nerrors += test_update_make_level2(fapl, &cparam2, &tparam);
if (TestExpress > 1)
if (localTestExpress > 1)
printf("***Express test mode on. test_update_lots skipped\n");
else
nerrors += test_update_lots(fapl, &cparam2, &tparam);
Expand All @@ -10006,7 +10008,7 @@ main(void)
nerrors += test_remove_level2_2internal_merge_right(fapl, &cparam, &tparam);
nerrors += test_remove_level2_3internal_merge(fapl, &cparam, &tparam);
nerrors += test_remove_level2_collapse_right(fapl, &cparam, &tparam);
if (TestExpress > 1)
if (localTestExpress > 1)
printf("***Express test mode on. test_remove_lots skipped\n");
else
nerrors += test_remove_lots(driver_name, fapl, &cparam);
Expand Down
4 changes: 2 additions & 2 deletions test/earray.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2302,8 +2302,8 @@ main(void)
/* Reset library */
h5_test_init();
fapl = h5_fileaccess();
if (TestExpress > 1)
printf("***Express test mode on. Some tests may be skipped\n");
if (TestExpress > 0)
printf("***Express test mode %d. Some tests may be skipped\n", TestExpress);
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to explicitly specify the type of printf value parameters (int here) because there is absolutely no type checking, and a change in the global variable type could cause hard to track down errors. The LocalTestExpress ones aren't as bad since that variable is declared in the function but it wouldn't hurt to add it there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you want here, since the variable is just an int?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cast LocalTestExpress to an int: "LocalTestExpress" => "(int)LocalTestExpress"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like a bad idea to cast something that doesn't really need casting since it'll just hide future possible errors.

Copy link
Member

Choose a reason for hiding this comment

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

If the type of the variable ever changes it can produce memory errors. Modern compilers might be able to detect and throw a warning in this case (if the type doesn't match the format string), I'm not sure.

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 think you want the compiler to report the issue and not cover it up?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not certain the compiler will report an issue if there's no cast. And it is much more unsafe to pass the wrong type to printf than to cast something to the right type - the only potential problem there is wrap around. printf looks at the format string and reads that many bytes from the input parameters, it does not check if enough data was passed in the parameters, so if the types don't match exactly it's possible for printf to read past the end of the input parameters which is a memory error.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like gcc does not warn by default but does if -Wformat is on

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we have Wformat on by default for GCC and most of the other compilers. My main concern is that casting to int can hide other problems if the variable ever does change to something else, but it seems unlikely that it will at this point. Absolutely not an important issue though; either way is fine.


/* Set the filename to use for this test (dependent on fapl) */
h5_fixname(FILENAME[0], fapl, filename_g, sizeof(filename_g));
Expand Down
4 changes: 2 additions & 2 deletions test/farray.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1633,8 +1633,8 @@ main(void)
/* Reset library */
h5_test_init();
fapl = h5_fileaccess();
if (TestExpress > 1)
printf("***Express test mode on. Some tests may be skipped\n");
if (TestExpress > 0)
printf("***Express test mode %d. Some tests may be skipped\n", TestExpress);

/* Set the filename to use for this test (dependent on fapl) */
h5_fixname(FILENAME[0], fapl, filename_g, sizeof(filename_g));
Expand Down
4 changes: 2 additions & 2 deletions test/fheap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15982,8 +15982,8 @@ main(void)
* Activate full testing when this feature is re-enabled
* in the future for parallel build.
*/
if (TestExpress > 1)
printf("***Express test mode on. Some tests may be skipped\n");
if (TestExpress > 0)
printf("***Express test mode %d. Some tests may be skipped\n", TestExpress);
else if (TestExpress == 0) {
#ifdef H5_HAVE_PARALLEL
num_pb_fs = NUM_PB_FS - 2;
Expand Down
4 changes: 2 additions & 2 deletions test/objcopy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17136,8 +17136,8 @@ main(void)
if (h5_driver_is_default_vfd_compatible(fapl, &driver_is_default_compatible) < 0)
TEST_ERROR;

if (TestExpress > 1)
printf("***Express test mode on. Some tests may be skipped\n");
if (TestExpress > 0)
printf("***Express test mode %d. Some tests may be skipped\n", TestExpress);

/* Copy the file access property list */
if ((fapl2 = H5Pcopy(fapl)) < 0)
Expand Down
4 changes: 2 additions & 2 deletions test/objcopy_ref.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1823,8 +1823,8 @@ main(void)
h5_test_init();
fapl = h5_fileaccess();

if (TestExpress > 1)
printf("***Express test mode on. Some tests may be skipped\n");
if (TestExpress > 0)
printf("***Express test mode %d. Some tests may be skipped\n", TestExpress);

/* Copy the file access property list */
if ((fapl2 = H5Pcopy(fapl)) < 0)
Expand Down
Loading