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 #397

Conversation

mwcondino
Copy link
Contributor

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.

@mwcondino mwcondino force-pushed the feature/mwcondino-support-warnings-as-errors branch from 382e730 to af6c1cb Compare July 26, 2022 19:13
@mwcondino
Copy link
Contributor Author

hey @mjeronimo, could you take a look when you get a chance?

@mwcondino mwcondino force-pushed the feature/mwcondino-support-warnings-as-errors branch from c82bd45 to ff51753 Compare July 26, 2022 20:57
@mhidalgo-bdai
Copy link

mhidalgo-bdai commented Apr 30, 2024

This looks like a small enough fix and I happen to have been hit by #411 too. @marcoag @clalancette do you think we could get this in any time soon?

@clalancette
Copy link
Contributor

This looks like a small enough fix and I happen to have been hit by #411 too. @marcoag @clalancette do you think we could get this in any time soon?

Yeah, this looks reasonable enough. I'm going to rebase it and then run CI on it.

Signed-off-by: Matt Condino <[email protected]>
Signed-off-by: Matt Condino <[email protected]>
Signed-off-by: Matt Condino <[email protected]>
@clalancette clalancette force-pushed the feature/mwcondino-support-warnings-as-errors branch from ff51753 to 9cbb36e Compare April 30, 2024 21:06
@clalancette
Copy link
Contributor

CI:

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

@clalancette clalancette merged commit f985e67 into ament:rolling Apr 30, 2024
3 checks passed
@mwcondino
Copy link
Contributor Author

Thanks for the bump @mhidalgo-bdai , and thanks for merging @clalancette!

@mwcondino mwcondino deleted the feature/mwcondino-support-warnings-as-errors branch April 30, 2024 21:40
@mhidalgo-bdai
Copy link

@clalancette thank you! Could we also ask the Mergify bot for a Humble backport?

@clalancette
Copy link
Contributor

@Mergifyio backport humble iron jazzy

Copy link

mergify bot commented May 1, 2024

mergify bot pushed a commit that referenced this pull request May 1, 2024
…n config (#397)

* feature

Signed-off-by: Matt Condino <[email protected]>
(cherry picked from commit f985e67)
mergify bot pushed a commit that referenced this pull request May 1, 2024
…n config (#397)

* feature

Signed-off-by: Matt Condino <[email protected]>
(cherry picked from commit f985e67)
mergify bot pushed a commit that referenced this pull request May 1, 2024
…n config (#397)

* feature

Signed-off-by: Matt Condino <[email protected]>
(cherry picked from commit f985e67)
ahcorde pushed a commit that referenced this pull request May 10, 2024
…n config (#397) (#489)

* feature

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

Co-authored-by: Matt Condino <[email protected]>
ahcorde pushed a commit that referenced this pull request May 10, 2024
…n config (#397) (#488)

* feature

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

Co-authored-by: Matt Condino <[email protected]>
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.

[ament_clang_tidy] Does not print errors and warnings in the terminal
3 participants