Conversation
|
@matyalatte can I continue code refactoring in this branch? you can delete unnecessary changes that you are not satisfied with. |
Can you run benchmark.yml in your fork when finishing refactoring? I can see the result without fetching your branch.
Yes, but it seems that your branch is failing unit tests, so it would be better to fix the bug first. You can run the tests with Ping me again when you've finished refactoring. |
matyalatte
left a comment
matyalatte
left a comment
There was a problem hiding this comment.
I've read the current code. Looks very good to me! I've posted some suggestions, so it would be better to read them before continuing refactoring.
include/options.h
Outdated
| static std::vector<fs::path> FilterExcludedFiles(std::vector<fs::path> filenames, | ||
| const std::vector<fs::path>& excludes); |
There was a problem hiding this comment.
It would be better to align arguments. I mean, insert a linefeed after ( or add more spaces before the second argument.
For example,
| static std::vector<fs::path> FilterExcludedFiles(std::vector<fs::path> filenames, | |
| const std::vector<fs::path>& excludes); | |
| static std::vector<fs::path> FilterExcludedFiles( | |
| std::vector<fs::path> filenames, | |
| const std::vector<fs::path>& excludes); |
Other lines using [[nodiscard]] and static also require similar modifications.
More info: - https://stackoverflow.com/questions/60784280/string-view-as-a-parameter-of-const-reference - https://stackoverflow.com/questions/57761077/return-const-value-prevent-move-semantics - https://stackoverflow.com/questions/62081977/why-is-there-no-optimization-for-checking-for-empty-string-via-comparison - https://stackoverflow.com/questions/37689228/difference-between-str-clear-and-str
@matyalatte how to run benchmark.yml actions on my fork, I have enabled Actions CI in my fork but it is empty.
|
|
Click on "Benchmark". There then should be a run button somewhere. |
@matyalatte https://github.com/GermanAizek/cpplint-cpp/actions/runs/10729927983/job/29757427250 |
|
You should probably fix the lint errors. |
matyalatte
left a comment
matyalatte
left a comment
There was a problem hiding this comment.
https://github.com/GermanAizek/cpplint-cpp/actions/runs/10729927983/job/29757427250
Thanks for the benchmarking. I confirmed that there are no performance issues. I'll merge this PR if some nit picks are fixed.
| std::vector<fs::path> FilterExcludedFiles(std::vector<fs::path> filenames, | ||
| const std::vector<fs::path>& excludes); | ||
| static std::vector<fs::path> FilterExcludedFiles( | ||
| std::vector<fs::path> filenames, |
There was a problem hiding this comment.
| std::vector<fs::path> filenames, | |
| std::vector<fs::path> filenames, |
Tab indentations should be removed.
|
|
||
| // Check if current position is inside template argument list. | ||
| bool InTemplateArgumentList(const CleansedLines& clean_lines, size_t linenum, size_t pos); | ||
| static bool InTemplateArgumentList(const CleansedLines& clean_lines, size_t linenum, size_t pos); |
There was a problem hiding this comment.
This should be split into two lines to pass the code check.
| static bool AddJUnitFailure(const std::string& filename, | ||
| size_t linenum, | ||
| const std::string& message, | ||
| const std::string& category, |
There was a problem hiding this comment.
You still need to align arguments for some functions that use static and [[nodiscard]].
| static bool IsMacroDefinition(const CleansedLines& clean_lines, | ||
| const std::string& elided_line, size_t linenum); |
| static bool ExpectingFunctionArgs(const CleansedLines& clean_lines, | ||
| const std::string& elided_line, size_t linenum); |
| [[nodiscard]] bool IsMatched(const std::string& category, | ||
| const std::string& file, | ||
| size_t linenum) const { |
3 participants
@matyalatte thank you so much for this project, I myself dreamed migrating this static linter to C++ or C, you fulfilled my dream. I would like to do my best to contributing.
My code optimization improvements, try to compare whether benchmark results on Release will improve with my branch.
References:
https://stackoverflow.com/questions/57761077/return-const-value-prevent-move-semantics
https://stackoverflow.com/questions/62081977/why-is-there-no-optimization-for-checking-for-empty-string-via-comparison
https://stackoverflow.com/questions/37689228/difference-between-str-clear-and-str
https://stackoverflow.com/questions/60784280/string-view-as-a-parameter-of-const-reference