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

Update gcc rules to 14.2 #2815

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Conversation

cmorve-te
Copy link
Contributor

@cmorve-te cmorve-te commented Nov 19, 2024

The Fortran-only aliasing, align-commons, ampersand, array-temporaries, character-truncation, conversion-extra, function-elimination, implicit-interface, implicit-procedure, intrinsic-shadow, intrinsics-std, line-truncation, real-q-constant, surprising, underflow and unused-dummy-argument warnings have been removed.

-Wsystem-headers has also been removed since it's not a warning, it's an option that affects whether warnings in system headers are reported.

-Wno-aggressive-loop-optimizations and -Wno-builtin-declaration-mismatch have been replaced with the correct -Waggressive-loop-optimizations and -Wbuiltin-declaration-mismatch.

I have not tried to keep the original format, since

  • Without a reference script would be difficult
  • It was not great

This is actually one I did even before #2814, but it was blocked by my employer's review process. I have just rebased it and done a quick sanity check, hopefully I have not missed anything.


This change is Reviewable

@guwirth guwirth self-requested a review November 20, 2024 07:51
@guwirth guwirth added this to the 2.2.0 milestone Nov 20, 2024
@guwirth
Copy link
Collaborator

guwirth commented Nov 20, 2024

@cmorve-te Normally, the sensors support all the rules of the current version and also those of the previous versions. I would not deviate from this in 2.1.x. With 2.2.x we can think about cleaning up and removing outdated rules (this PR is for 2.2). For me, the decisive factor is always which version of the tool is included in larger Linux distributions. These rules should at least be supported.

@cmorve-te
Copy link
Contributor Author

To be clear, do you want me to modify this PR? If so, there are

a) Fortran-only warnings
b) Not-a-real-warning "system-headers"
c) Two wrong "-Wno" versions

'a' has been generated by gcc, but only for Fortran programs
'b' and 'c' have never been generated by gcc

Do you want me to keep all of them, even the wrong ones?

Copy link
Collaborator

@guwirth guwirth left a comment

Choose a reason for hiding this comment

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

see comments

@@ -115,7 +115,21 @@ protected String alignId(@Nullable String id) {
if (id == null || "".equals(id)) {
id = DEFAULT_ID;
}
return id.replaceAll("=$", "");
id = id.replaceAll("=$", "");
Copy link
Collaborator

@guwirth guwirth Nov 20, 2024

Choose a reason for hiding this comment

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

regex are slow, could also be:

if (id.charAt(id.length() - 1) == '=') {
        id = id.substring(0, str.length() - 1);
}

return id.replaceAll("=$", "");
id = id.replaceAll("=$", "");

if (id.equals("-Wc++0x-compat")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

chain of if/if else: would use a switch

@guwirth
Copy link
Collaborator

guwirth commented Nov 20, 2024

To be clear, do you want me to modify this PR?

@cmorve-te No

I like to verify which gcc version is part of current CI/CD systems

  • Ubuntu 22.04.5 LTS
  • Debian 11 “Bullseye”

In case it's an older one we should discuss if it makes sense to support also these old rules?
I will investigate if there are pre-installed packages (or maybe you know it).

@cmorve-te
Copy link
Contributor Author

Ubuntu 22.04.5 LTS

11.2.0 -> https://packages.ubuntu.com/jammy/gcc

Debian 11 “Bullseye”

10.2.1 -> https://packages.debian.org/bullseye/gcc

In case it's an older one we should discuss if it makes sense to support also these old rules?

There is the embedded world, which has a tendency to use old versions. Some references

@guwirth
Copy link
Collaborator

guwirth commented Nov 20, 2024

@cmorve-te same here. All rules starting from 10.2.1 should be in the XML, older rules can be deleted.

@cmorve-te
Copy link
Contributor Author

All rules starting from 10.2.1 should be in the XML, older rules can be deleted.

I would rather keep anything down to gcc 5.0. As mentioned, it's not an uncommon version in the embedded world, and we specifically do build with it.

The Fortran-only aliasing, align-commons, ampersand, array-temporaries,
character-truncation, conversion-extra, function-elimination,
implicit-interface, implicit-procedure, intrinsic-shadow,
intrinsics-std, line-truncation, real-q-constant, surprising, underflow
and unused-dummy-argument warnings have been removed.

-Wsystem-headers has also been removed since it's not a warning, it's an
option that affects whether warnings in system headers are reported.

-Wno-aggressive-loop-optimizations and -Wno-builtin-declaration-mismatch
have been replaced with the correct -Waggressive-loop-optimizations and
-Wbuiltin-declaration-mismatch.
I have not actually measured, but regexs are slow.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hope did the comparision this time right. Below were in the old XML but no more in the new one:

-Waliasing
-Walign-commons
-Wampersand
-Warray-temporaries
-Wc++0x-compat
-Wc++1z-compat
-Wcharacter-truncation
-Wconversion-extra
-Wfunction-elimination
-Wimplicit-interface
-Wimplicit-procedure
-Wintrinsic-shadow
-Wintrinsics-std
-Wline-truncation
-Wmissing-format-attribute
-Wmissing-noreturn
-Wno-aggressive-loop-optimizations
-Wno-builtin-declaration-mismatch
-Wreal-q-constant
-Wsurprising
-Wsystem-headers
-Wunderflow
-Wunused-dummy-argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, all expected

-Waliasing (Fortran-only)
-Walign-commons (Fortran-only)
-Wampersand (Fortran-only)
-Warray-temporaries (Fortran-only)
-Wc++0x-compat (alias for -Wc++11-compat)
-Wc++1z-compat (alias for -Wc++17-compat)
-Wcharacter-truncation (Fortran-only)
-Wconversion-extra (Fortran-only)
-Wfunction-elimination (Fortran-only)
-Wimplicit-interface (Fortran-only)
-Wimplicit-procedure (Fortran-only)
-Wintrinsic-shadow (Fortran-only)
-Wintrinsics-std (Fortran-only)
-Wline-truncation (Fortran-only)
-Wmissing-format-attribute (alias for -Wsuggest-attribute=format)
-Wmissing-noreturn (alias for -Wsuggest-attribute=noreturn)
-Wno-aggressive-loop-optimizations (wrong, it's -Waggressive-loop-optimizations)
-Wno-builtin-declaration-mismatch (wrong, it's -Wbuiltin-declaration-mismatch)
-Wreal-q-constant (Fortran-only)
-Wsurprising (Fortran-only)
-Wsystem-headers (not a real warning)
-Wunderflow (Fortran-only)
-Wunused-dummy-argument (Fortran-only)

I left the Objective-C/Objective-C++-only warnings. But didn't include the Fortran-only since it's seemed too unrelated for a C++ plugin? The warnings are documented literally in a different place, e.g. https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gfortran/Error-and-Warning-Options.html#index-Waliasing (vs https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Warning-Options.html).

I am not familiar with Fortran, though. Happy to readd them if you would rather keep them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cmorve-te thx then we can merge it.

Do you like to fix the 2 findings above or should I do it as part of technical debt clean-up?

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 have the changes. I just wanted to test them first, and got busy with something else. Will try to push them later today.

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'm not going to get the chance to test it, but I have pushed it anyway.
I am not a Java guy, to the point I didn't know you could use switch with strings... but I want to think I have not been able to break this: https://github.com/SonarOpenCommunity/sonar-cxx/compare/b024e9c3748d7c16c58edd5093b7b00969c4b489..0c7b6c307393de5b08c02b16fe6cc93e465a000c.

@guwirth guwirth merged commit 24f35c7 into SonarOpenCommunity:master Nov 22, 2024
12 checks passed
@guwirth
Copy link
Collaborator

guwirth commented Nov 22, 2024

@cmorve-te thx for providing this PR

guwirth added a commit to guwirth/sonar-cxx that referenced this pull request Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants