Conversation
|
@akuker In order not to lose momentum I have already started a second round of addressing SonarCloud issues. There is a new fix_sonarcloud_issues2 (based on fix_sonarcloud_issues). In case you have not yet started reviewing fix_sonarcloud_issues I would like to merge fix_sonarcloud_issues2 into fix_sonarcloud_issues. With the right setup (Homebrew for protobuf and stdlog) rasctl now compiles and runs on macos (Intel), by the way. |
|
@akuker Regarding the emails with error reports I get, my impression is that they are in particular sent when something is committted while an action for a previous commit is still runing. Not absolutely sure, though. |
|
@akuker Just figured out how to get the compile-time logs. The problem is a compile-time problem only visible with -O3. I will try to find a solution, till then I will put the PR back into draft status. |
|
Fixed the compile time error. It was related to a SonarQube (strcpy/strncpy) issue, which may have to be resolved later. |
|
Up to date SonarCloud results are now available for this PR. |
| { | ||
| const AbstractController *controller = FindController(id); | ||
| if (controller != nullptr) { | ||
| if (const auto controller = FindController(id); controller != nullptr) { |
There was a problem hiding this comment.
Could this be written like this?
if (FindController(id) != nullptr)
There was a problem hiding this comment.
This would not work, because we need the controller in the next line. This is the complete context:
if (const auto controller = FindController(id); controller != nullptr) {
return controller->GetDeviceForLun(lun);
}
return nullptr;
|
Lots of changes! Thank you for looking into these, @uweseimet . Just a couple comments. |
|
@akuker Thank you for reviewing. I am quite sure that from my side this PR is the last one with so many files being changed in a single PR. The other tickets (even those with major effort) would not affect essentially everything. I hope I am not making wrong promises ;-) And provided that there are not too many issues of this kind still being reported. I don't know why, but the issues reported by SonarLint within Eclipse do not always match those reported in the SonarCloud UI, even though SonarLint connects to the SonarCloud server and gets its settings from this server. Some changes probably take getting used to, at least for me, because they are using C++ features I was not yet familiar with. When looking at the reasons why SonarCloud made these suggestions, and when looking at the C++ standard, they made a lot of sense. A lot of them reduce the danger of side effects. And some, like structured bindings, make the code much more explicit. |
|
SonarCloud Quality Gate failed. |
There might be settings we need to tweak in SonarCloud. I think I added you to the project, but I'm not sure if that gives you access to the settings or not.
These new features of C++ are new to me as well. I learned quite a bit from reviewing your changes, so thank you! :) As a former Ada programmer, the |
|
@akuker auto is similar to var/val etc. in other languages, and I do not use it everywhere. SonarCloud also does not propose to use it everywhere. It suggests to use it only when you can immediately see the type from the rvalue. |
* Fixing SonarCloud issues, first round * Fixing SonarCLoud issues, next round * Fixing SonarQube issues, next round * Fixed warning * Replaced empty constructors/destructors with = default; * Fixed warning * Replaced new * Use constants instead of macros * Use structured binding declarations * Use init statements for if * Use string views * Use enum class, use using instead of typedef * Fixed more SonarCloud warnings * Replaced redundant/duplicate types with auto * Devlared methods const * Memory management update * Fixed warning * Added missing const * Improved RaScsiResponse memory management * Improved memory management * Improved memory management * Replaced macros by constants, removed unused constants * Made member private * Fixed warning * Added comment * Fixed shadowing warnings * Cleanup * Cleanup * Cleanup * Fixed shadowing warning * Removed unused code * Fixed more warnings * Removed obsolete casts * Fixed warnings * Removed unused field * Removed library not needed by rasctl * Include cleanup * Updated platform check for better compatibility * Improved check for invalid option. This prevents rasctl to break on macos. * Updated option check * Fixed typo * Added TODO * Removed macro * Scope update * Replaced macro * Added TODO, update memory management * Fixed typo * Replaced NULL by nullptr * Use more structured bindings * Added TODO * Use calloc instead of mallco to not need memset * Fixed warnings * Fixed SonarQube initialization issues * Fixed warning * Cleaned up override/virtual/final * Fixed warnings * Constructor update * Fixed tests * Improved memory management * Added missing const * Added const * Fixed two bugs reported by SonarCloud * Fix SonarCloud hotspot * Fixed memory management * Memory management update * Addressing hotspot by using strncpy * Fixed SonarCloud issues * Fixed SonarQube issues * Added missing const * Added const * Added const * Suppress false positive * Added SonarQube suppressions for false positives * Added suppresoin * Fixed code smells * Reverted changes that is a SonarQube issue, but caused problems with -O3 * Removed TODO based on review
|
@akuker Regarding SonarCloud settings, being a project member is not sufficient. I would need to be able to create new quaiity profiles. |
* Added support for .hd1 (SCSI-1) image files * Update c-cpp.yml * Fixed unit test warnings * Fixed wrong scan default default (must be 1, not -1) * Updated max length check * Removed file not present in develop branch * Added unit test * Added workflow configurations and README updates (#832) * automated test try 1 * filter branches * filter branches * filter branches * filter branches * filter branches * Configured build and test CI workflows * enable for all branches * Update README.md * Update README.md Co-authored-by: Tony Kuker <akuker@gmail.com> * Fix simple SonarCloud issues (#834) * Fixing SonarCloud issues, first round * Fixing SonarCLoud issues, next round * Fixing SonarQube issues, next round * Fixed warning * Replaced empty constructors/destructors with = default; * Fixed warning * Replaced new * Use constants instead of macros * Use structured binding declarations * Use init statements for if * Use string views * Use enum class, use using instead of typedef * Fixed more SonarCloud warnings * Replaced redundant/duplicate types with auto * Devlared methods const * Memory management update * Fixed warning * Added missing const * Improved RaScsiResponse memory management * Improved memory management * Improved memory management * Replaced macros by constants, removed unused constants * Made member private * Fixed warning * Added comment * Fixed shadowing warnings * Cleanup * Cleanup * Cleanup * Fixed shadowing warning * Removed unused code * Fixed more warnings * Removed obsolete casts * Fixed warnings * Removed unused field * Removed library not needed by rasctl * Include cleanup * Updated platform check for better compatibility * Improved check for invalid option. This prevents rasctl to break on macos. * Updated option check * Fixed typo * Added TODO * Removed macro * Scope update * Replaced macro * Added TODO, update memory management * Fixed typo * Replaced NULL by nullptr * Use more structured bindings * Added TODO * Use calloc instead of mallco to not need memset * Fixed warnings * Fixed SonarQube initialization issues * Fixed warning * Cleaned up override/virtual/final * Fixed warnings * Constructor update * Fixed tests * Improved memory management * Added missing const * Added const * Fixed two bugs reported by SonarCloud * Fix SonarCloud hotspot * Fixed memory management * Memory management update * Addressing hotspot by using strncpy * Fixed SonarCloud issues * Fixed SonarQube issues * Added missing const * Added const * Added const * Suppress false positive * Added SonarQube suppressions for false positives * Added suppresoin * Fixed code smells * Reverted changes that is a SonarQube issue, but caused problems with -O3 * Removed TODO based on review * Update c-cpp.yml * Finalized merge Co-authored-by: akuker <34318535+akuker@users.noreply.github.com> Co-authored-by: Tony Kuker <akuker@gmail.com>
This PR fixes SonarQube issues where the resolution appeared obvious to me and required no refactoring: Missing const, nullptr instead of NULL, empty methods, missing default cases etc.
There are a lot of remaining issues that require closer investigation, but also many that appear to be false positives, e.g. related to constness. This is amazing because with other languages false positives are extremely rare. This also needs further investigation.
@akuker I would like to ask for permission to change the SonarQube rule settings (for C++). I guess you have to configure this. Over time I intend to make the rules stricter, but for now I would just like to switch off the rule that complain about TODOs, for instance. We know that we have TODOs anyway, and the IDE already points them out.