Unidata / UDUNITS-2

API and utility for arithmetic manipulation of units of physical quantities
http://www.unidata.ucar.edu/software/udunits
Other
62 stars 36 forks source link

testUnits.c: Add return type to test_timeResolution #100

Closed schwehr closed 3 years ago

schwehr commented 3 years ago

With strict compiler settings:


third_party/udunits/lib/testUnits.c:2290:1: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
test_timeResolution(void)
^
third_party/udunits/lib/testUnits.c:2310:1: error: non-void function does not return a value [-Werror,-Wreturn-type]
}
^
``
semmerson commented 3 years ago

Already fixed. Working on getting the latest version out. We changed our CI/CD pipeline :-(

schwehr commented 3 years ago
  1. I'm using a bazel based build, so if there was a change to the CI/CD pipeline that stops using this file, I won't see that.
  2. I am building from head, so are you talking about a fix that is out-of-band with respect to the master branch on github?
  3. If this file is no longer used for testing, then why not remove it?
  4. Please explain what you mean by a change in the CI/CD pipeline.

Please merge this fix. The error is still in testUnits.c at head:

https://github.com/Unidata/UDUNITS-2/blob/3e607e13e8964e21e358690fa87b5e8a03b140e1/lib/testUnits.c#L2284

    ut_free(unit1);
    ut_free(unit2);
    ut_free_system(xmlSystem);
}

test_timeResolution(void)
{
    ut_system*  xmlSystem;

    ut_set_error_message_handler(ut_write_to_stderr);
semmerson commented 3 years ago
  1. I'm using a bazel based build, so if there was a change to the CI/CD pipeline that stops using this file, I won't see that.

  2. If this file is no longer used for testing, then why not remove it?

The file is still being used.

  1. Please explain what you mean by a change in the CI/CD pipeline.

We're migrating away from Travis because of their usage quotas; Consequently, we're having to recreate our CI/CD pipelines. That's what I'm currently working on.

If your actions are triggered by a push to the main branch, then you should still be Ok. As soon as the pipeline is set, I'll push to the main branch.

schwehr commented 3 years ago

I don't see how this fix has anything to do with a change with CI/CD and Travis. Are you saying:

  1. that you no longer see a complaint because you've loosened your checks for which ever CI/CD you are switching to?

Or

  1. that you've got a patch somewhere else that is on the way and you'd rather not have to handle the merge resolution of the fix coming from two different branches?

If 1, with the compiler and build flags I use, the lack of a void return type is an error. It would be great to have this fix in. For me, it would be great to not have to carry another local patch. For everyone else, test_timeResolution's return is unlike every other test function in this file and having uniform function prototypes is a good thing to reduce confusion.

schwehr commented 3 years ago

Never mind. I figured out that it's "2." above... I see that you've got the change here:

https://github.com/Unidata/UDUNITS-2/commit/cc59096124699644622f623941921644334014f0#diff-bc4deac4c1192d35f39f48e9ebf38b6a51e2617c40e57d5a8fddeff388fb4cd7R2281