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

Fix compiler errors that were not caught by GCC #206

Closed
wants to merge 3 commits into from

Conversation

adamlm
Copy link
Contributor

@adamlm adamlm commented Jan 4, 2024

PR Details

Description

This PR adds a Clang-Tidy configuration to help improve our software quality with static analysis. When running the tool, clang-tidy reported a compiler error that was not caught by the GCC version we are using. The GCC version we currently use has a bug that was patched in newer versions. Clang (which clang-tidy uses internally) does not have this bug.

Related GitHub Issue

Closes #205

Related Jira Key

Closes CDAR-674

Motivation and Context

Compilation error

How Has This Been Tested?

N/A, compiler errors

Types of changes

  • Defect fix (non-breaking change that fixes an issue)

Checklist:

@adamlm adamlm self-assigned this Jan 4, 2024
@adamlm adamlm requested a review from JonSmet January 4, 2024 14:23
@JonSmet
Copy link
Contributor

JonSmet commented Jan 4, 2024

Just putting a note here since we discussed it verbally, but let's take some time to think about how we can clone/checkout the carma_ament_lint repo at the start of the build process for our repos that depend on carma-utils, since this will be required to enable the carma_ament_cmake_clang_tidy package to be built later on. With our current approach, we'd have to add the repo to the checkout.bash script of all repos that depend on carma-utils packages, which is a lot.

@adamlm
Copy link
Contributor Author

adamlm commented Jan 22, 2024

@JonSmet I found that clang-tidy-6.0 comes with Ubuntu 20.04, so we don't have to rely on the generic clang-tidy binary. I removed the dependency on our custom carma_cmake_clang_tidy because of this.

There is a remaining question: Should we include a Clang-Tidy dependency on this PR? Even if we don't include it, there's is still a compilation error that needs to be resolved. We can push Clang-Tidy integration to another PR. Thoughts?

This will help us lint our code.
The CarmaLifecycleNode has some compilation errors that were not
caught by GCC due to a bug in the compiler. Clang-Tidy uses the Clang
compiler, which did not have that bug. Newer versions of GCC are
patched.
It turns out that clang-tidy-6.0 is available on Ubuntu 20.04. Not
sure why I didn't catch that. We can replace the CARMA specific
ament clang-tidy integration with the one from ROS. There is still the
issue of colcon test-result not showing clang-tidy's outputs, but
there are other ways around that.
@adamlm
Copy link
Contributor Author

adamlm commented Jan 22, 2024

@adamlm adamlm closed this Jan 22, 2024
adamlm added a commit that referenced this pull request Jan 22, 2024
# PR Details
## Description

This PR fixes compiliation errors that were not caught by our version of
GCC due to a compiler bug Clang-Tidy uses the Clang compiler, which did
not have that bug. Newer versions of GCC are patched.

> [!NOTE]
> This PR cherry-picks relevant commits from #206 into a more focused
change set.

## Related GitHub Issue

Closes #205 

## Related Jira Key

Closes [CDAR-674](https://usdot-carma.atlassian.net/browse/CDAR-674)

## Motivation and Context

These changes resolve errors that should have prevented the package from
compiling.

## How Has This Been Tested?

Built the package.

## Types of changes

- [x] Defect fix (non-breaking change that fixes an issue)

## Checklist:

- [x] I have read the
[**CONTRIBUTING**](https://github.com/usdot-fhwa-stol/carma-platform/blob/develop/Contributing.md)
document.


[CDAR-674]:
https://usdot-carma.atlassian.net/browse/CDAR-674?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
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.

CarmaLifecycleNode default template parameter values redefine existing default parameter
2 participants