[feat] Add memory safety reporter script - #4962
Conversation
gulyasgergely902
added
the
tools 🛠️
b26bda3 to
2d690d6
Compare
21feff0 to
1e403e1
Compare
barnabasdomozi
left a comment
barnabasdomozi
left a comment
There was a problem hiding this comment.
Can you add a test case which uses this script?
In the future, flags for these CodeChecker commands could change and then this script could be broken by accident.
10f70cb to
b4cd671
Compare
barnabasdomozi
left a comment
barnabasdomozi
left a comment
There was a problem hiding this comment.
As mentioned in my other comment, please add a simple test case which is executed in the GitHub CI.
089d751 to
d8d91bb
Compare
barnabasdomozi
left a comment
barnabasdomozi
left a comment
There was a problem hiding this comment.
I think creating a whole python package with setup.py and Makefile is unnecessary for this simple script. And I also think it's unnecessary to add Makefile targets to upload this to the PyPi registry.
Moreover, when building CodeChecker with
make venv and then make package, this new memory-safety-reporter command is not available for some reason.
As a simpler solution, I think it would suffice to add #!/usr/bin/env python3 to the top of the file, and then it could be executed as memory-safety-reporter.
An alternative would be to add this script as a CodeChecker subcommand, e.g. CodeChecker memory-safety-converter. I think this would make more sense and it would worth a discussion with the team.
As for the test cases, the tools/memory-safety-reporter/tests/functional/cmdline/test_files/ directory should be generated by the test case and not commited to this repository. Ideally, the test case should not include binary files like tools/memory-safety-reporter/tests/functional/cmdline/test_files/test-report/failed/xmltest.cpp_clangsa_382c4897e0fb3b0c4fe94ece86cee8ca.plist_CTU_compile_error.zip and should also not include development machine absolute paths like /local/test_projects/tinyxml2/xmltest.cpp.
|
After a short meeting agreed to:
|
f05ea91 to
5266e55
Compare
barnabasdomozi
left a comment
barnabasdomozi
left a comment
There was a problem hiding this comment.
Overall looks good to me, but I left a few comments.
|
|
||
| package: package_dir_structure set_git_commit_template package_gerrit_skiplist | ||
| package_memory_safety_reporter: package_dir_structure | ||
| cp -p tools/memory-safety-reporter/memory_safety_reporter.py $(CC_BUILD_BIN_DIR) |
There was a problem hiding this comment.
Minor suggestion:
I think it would be more clean if we copy this python file to the bin folder and name it memor_safety_reporter (without the .py).
This way, users can just call it by typing memory_safety_reporter.
There was a problem hiding this comment.
Please remove these large .plist and .plist.bak files from this patch. As an example, this file is 4500 lines long.
I think we should keep these test cases as small as possible with only few plists files that are below 100 lines.
There was a problem hiding this comment.
I've created an even smaller report by analyzing a single cpp file. This contains only the necessary files to test the script.
7bce7de to
a6f1d27
Compare
240073a to
44b0ca7
Compare
This change adds the new memory safety reporter script which collects all configs which were used for a given analysis.
44b0ca7 to
09e5f09
Compare
3 participants
This change adds the new memory safety reporter script which collects all configs which were used for a given analysis.