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

Improve condition for declaring struct timeval #14540

Merged
merged 1 commit into from
Apr 28, 2021
Merged

Improve condition for declaring struct timeval #14540

merged 1 commit into from
Apr 28, 2021

Conversation

mjbogusz
Copy link
Contributor

Summary of changes

Improve condition for declaring struct timeval - do it only if it wasn't declared before.

This fixes usage of clang-tidy.

Without this fix, I'm hitting errors like this when using clang-tidy (paths slightly redacted):

/path-to-my-code/mbed-os/platform/include/platform/mbed_rtc_time.h:37:8: error: redefinition of 'timeval' [clang-diagnostic-error]
struct timeval {
       ^
/opt/gcc-arm-none-eabi/arm-none-eabi/include/sys/_timeval.h:54:8: note: previous definition is here
struct timeval {
       ^

Impact of changes

Migration actions required

Documentation


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Apr 13, 2021
@ciarmcom ciarmcom requested a review from a team April 13, 2021 14:30
@ciarmcom
Copy link
Member

@mjbogusz, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

/* Timeval definition for non GCC_ARM toolchains,
* note: _TIMEVAL_DEFINED is set e.g. in arm-none-eabi/include/sys/_timeval.h
*/
#if !defined(__GNUC__) && !defined(_TIMEVAL_DEFINED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing __clang__ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it was causing the double-defined errors when using clang-tidy.
AFAIK clang is not supported as a compiler for Mbed anyway, so it shouldn't break anything, but this change allows usage of clang-tidy on a compilation database generated for the GCC toolchain.

I've tried to check for defines used in the headers declaring the struct, but there is no consistency between them, so the alternative solution would be to cover all the known define flags and decide whether do define struct timeval based on that.
The defines I've found so far are:

  • The libc and derivatives (e.g. Ubuntu/Debian package libc6-dev) provides #define __timeval_defined 1, e.g. in /usr/include/<architecture_specific>/bits/types/struct_timeval.h
  • The newlib (e.g. package libnewlib-dev) provides #define _TIMEVAL_DEFINED, e.g. in /usr/include/newlib/sys/_timeval.h
    I don't have ARM's microlib at hand to inspect what defines it does provide.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we define our own, and check if not yet defined (_TIMEVAL_DEFINED), we should define it.
adding on line 39 #define _TIMEVAL_DEFINED so anyone getting this header, can check if its defined and it is (by us)

Copy link
Contributor

Choose a reason for hiding this comment

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

we define gnu flag for compatibility for ARMClang, so __GNUC__ is defined. I assume ARMClang do not have defined _TIMEVAL_DEFINED (seeing similar implementation https://github.com/cesanta/mjs/blob/master/mjs.c#L1054 for mbed).

I'll fetch this to test with ARMClang, I cant quickly find in the online docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the most compatible solution would be to drop the defined(__GNUC__) check whatsoever and decide only based on the existance of the _TIMEVAL_DEFINED (and its variants)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so then. I'll check if we use in tests timeval so our CI tests this with both toolchains we support.

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've pushed a version with checking for the defines - let me know if there are any more conditions we should check.

@0xc0170 0xc0170 requested a review from a team April 22, 2021 12:21
@mergify mergify bot added needs: CI and removed needs: review labels Apr 22, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 23, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 23, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit a32a45c into ARMmbed:master Apr 28, 2021
@mergify mergify bot removed the ready for merge label Apr 28, 2021
@mjbogusz mjbogusz deleted the fix-clang-timeval branch April 28, 2021 14:35
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants