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

ament_clang_tidy - Fix Reporting when WarningsAsErrors is specified in config (backport #397) #490

Open
wants to merge 1 commit into
base: jazzy
Choose a base branch
from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented May 1, 2024

Issue Summary

When WarningsAsErrors is specified in .clang-tidy, this can lead to some failures in result reporting. Specifically, the following test program (src/factorial.cpp):

int Factorial(int n) {
    int result = 1;
    int fake = 1;
    for (int i = 1; i <= n; i++) {
        result *= i;
    }
    return result;
}

fails with this output:

The invocation of "clang-tidy" failed with error code 1: Command '['/usr/bin/clang-tidy', '-p', 'path/test_examples_cmake', '--header-filter', 'include/test_examples_cmake/.*', 'path']' returned non-zero exit status 1.

when running ament_clang_tidy.

After applying this fix, I get the following output, which is expected

src/factorial.cpp:36:9: error: unused variable 'fake' [clang-diagnostic-unused-variable,-warnings-as-errors]
    int fake = 1;
        ^

Relevant section of .clang-tidy, for reference:

Checks:
    'bugprone*,
     clang-analyzer*,
     cert*,
     performance*,
     portability*,
     readability*,
     hicpp*,
     -hicpp-signed-bitwise,
     google-readability-todo,
    '
FormatStyle:     file
WarningsAsErrors: '*'

Fix

By trying to access the output attribute of the raised exception, we can populate the output in cases where exceptions were raised and there was valid output.


This is an automatic backport of pull request #397 done by [Mergify](https://mergify.com).

…n config (#397)

* feature

Signed-off-by: Matt Condino <[email protected]>
(cherry picked from commit f985e67)
@sloretz
Copy link
Contributor

sloretz commented May 9, 2024

Assigned @clalancette to run CI and merge 🧇

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

I'm going to suggest we hold off on this one until after the Jazzy release. It is low-risk, but also low-impact, so I think it is fine to put it in for a patch release.

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.

5 participants