-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
SASI code removal, error handling update, bug fixes, code cleanup #806
Conversation
@akuker I just updated my first comment in order to provide more details on the changes. This will hopefully help with the review. |
@akuker By accident I committed one change (adding -lpthread) to scsi_removal instead of scsi_removal_cleanup, I'm sorry for that ... |
No problem. Thank you for letting me know!! FYI: I'm going to dedicate some time to this PR this weekend. @uweseimet - side question - do you have a configuration/profile for code stye/formatting that you've been using? I'm wondering about committing a clang-format or uncrustify configuration file for the C++ side of things. (Uncrustify would be a lot smaller impact to the pre-built image (488k) vs clang-format wanted to pull in 135MB of stuff. |
@akuker Thank you for spending your time on the PR this weekd. Regarding formatting, I dont'use a special formatting. I just tried to format the code manually in a way that I thought was used in most of the existing code. Regarding the usage of tabs vs. blanks and indentation Eclipse appears to look at the existing file and handles this automatically. Emacs also does it like this as far as I know. I would be fine with reformatting the code in a consistent way, but it should be a format the usual IDEs (Eclipse in particular) support, so that once the code was reformatted the IDE preserves it. Otherwise things become very inconvenient, as I learned in a project where different IDE were used and it was not possible to configure these IDEs to use the same formatting rules. I'm not sure what you mean with an impact on the pre-built image, though. |
With every release, we put out a Raspberry Pi OS image that can be directly flashed to an SD card. I'd like to keep that as small and light-weight as possible. Whatever tool we use, I'm assuming we'd want to include it in the release disk image. The last image is at the bottom of this page: https://github.com/akuker/RASCSI/releases/tag/v22.08.01 |
@akuker I'm afraid I don't see the connection between code formatting and the image. My guess now is that the image contains the sources and you would like to consistently format the code that is added to the image. Not necessarily the code that is in git. Am I right? In this case I cannot provide any useful feedback because I have never used external formatting tools. Eclipse (at least for Java sources, not sure about C++) optionally automatically formats the code according to the configured style when saving a source file. In order to consistently format a project's (Java) sources, you typcially format everything with Eclipse once iin the configured format. Then you ensure that you have enabled the format-on-save option. This way the formatting stays consistent without any additional actions. |
Sorry... I'm not articulating my thoughts very well. I want to add the C++ code formatting as part of either the git commit hook or as part of the 'nightly' build process. It really has very little to do with the image. I just want to pick a tool that is reasonably small so we can include it in the default image. After a little more research, astyle looks like a potentially good option. There is an ecplise plugin for it, and also a VS Code plugin. I'm getting ahead of myself though - lets get this PR done first. |
@akuker Using git commit hooks would likely cause issues because usually you have open editors in your IDE or editor. If code is reformatted during the commit, the code in the filesystem (modified during the commit) differs from the code in the editors. This would trigger IDE or editor warnings for any file that has changed externally, This can be very annoying. It's as if a third person takes away control of your files and modifies them while you are editing. |
Looks good! Just a couple comments/questions. Thank you @uweseimet !! |
@akuker Thank you for reviewing! Please note that there is a sasi_removal_cleanup branch with (only a few lines) related changes. In the ideal case these changes would be merged into sasi_removal before merging everything together. Would be great if you could have a look at these changes. I just merged develop into sasi_removal. |
I took a look and have no comments. feel free to merge them into this PR, then I'll approve.
So, maybe I should get some static analysis set up? ;-) |
@akuker I am goint to merge sasi_removal_cleanup and will report back when I am done. Regarding code analysis I guess you already know that I would like this ;-). Relying on compier warnings (even with -pedantic or -Weffc++) only is not really helpful anymore, because there are hardly any warnings left. And I expect a static analysis to find more valuable (and trickier to address) issues. At least this is the case with tools like SonarQube, which I use for Java and Kotlin anaysis. It's just a matter of time until these warnings are also gone, because you learn where the issues are, why they are issues, and how to avoid them before checking in. |
@akuker Merge done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved! Thanks!
@uweseimet - can we delete this branch? Or did you want to continue developing on it? |
@akuker I would like to keep it for at least some days, in case an issue pops up where individual commits might be relevant. |
Sounds good. Thanks! |
After approving, I would like to merge into develop myself, because I may want to add some comments to the commit message.
The most important changes:
gcov usage example (make flags must include --coverage):
Or nicer with lcov instead of gcov:
There is a new Makefile 'coverage' target combining all these steps. The folder with the generated HTML is 'coverage'.
At least temporarily up to date code coverage results for a selected branch are published regularly.
@akuker Hope you are done with automation soon!