-
Notifications
You must be signed in to change notification settings - Fork 44
add cpp linter (clang-tidy) workflow #372
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
Changes from all commits
fe35f17
9937c73
075ed3a
0009b24
7e9de88
d4e4006
95e5bac
992548d
f2e9daf
e59c326
3349025
ff146d7
a4c7bf7
7650d95
723574d
0e60c82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,3 +19,4 @@ jobs: | |
| lgtm_comment_body: '' | ||
| annotations: false | ||
| max_comments: 10 | ||
| num_comments_as_exitcode: false | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| name: clang-tidy-review | ||
|
|
||
| # You can be more specific, but it currently only works on pull requests | ||
| on: | ||
| pull_request: | ||
| paths: ['**.cpp', '**.h', '**.hpp', '**CMakeLists.txt', '**.cmake', '.github/workflows/clang*.yml'] | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v5 | ||
|
|
||
| - uses: ZedThree/[email protected] | ||
| id: review | ||
| with: | ||
| split_workflow: true | ||
| build_dir: build | ||
| apt_packages: "libsnappy-dev, libzzip-dev, zlib1g-dev, libboost-all-dev, libzstd-dev" | ||
| clang_tidy_checks: '' | ||
| cmake_command: "cmake -Bbuild -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ." | ||
|
|
||
| - uses: ZedThree/clang-tidy-review/[email protected] | ||
|
|
||
| # If there are any comments, fail the check | ||
| - if: steps.review.outputs.total_comments > 0 | ||
| run: exit 1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| #include <vector> | ||
|
|
||
| #include "keyvi/dictionary/fsa/automata.h" | ||
| #include "keyvi/dictionary/fsa/traversal/traversal_base.h" | ||
| #include "keyvi/dictionary/fsa/traverser_types.h" | ||
|
|
||
| // #define ENABLE_TRACING | ||
|
|
@@ -65,7 +66,7 @@ class ComparableStateTraverser final { | |
| using label_t = typename innerTraverserType::label_t; | ||
| using transition_t = typename innerTraverserType::transition_t; | ||
|
|
||
| explicit ComparableStateTraverser(const innerTraverserType &&traverser, const bool advance = true, | ||
| explicit ComparableStateTraverser(const innerTraverserType&& traverser, const bool advance = true, | ||
| const size_t order = 0) | ||
| : state_traverser_(std::move(traverser)), order_(order) { | ||
| if (advance) { | ||
|
|
@@ -85,7 +86,7 @@ class ComparableStateTraverser final { | |
| : ComparableStateTraverser(f, f->GetStartState(), advance, order) {} | ||
|
|
||
| explicit ComparableStateTraverser(const automata_t f, const uint64_t start_state, | ||
| traversal::TraversalPayload<transition_t> &&payload, const bool advance = true, | ||
| traversal::TraversalPayload<transition_t>&& payload, const bool advance = true, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "keyvi::dictionary::fsa::traversal::TraversalPayload" is directly included [misc-include-cleaner] keyvi/include/keyvi/dictionary/fsa/comparable_state_traverser.h:33: - #include "keyvi/dictionary/fsa/traverser_types.h"
+ #include "keyvi/dictionary/fsa/traversal/traversal_base.h"
+ #include "keyvi/dictionary/fsa/traverser_types.h" |
||
| const size_t order = 0) | ||
| : state_traverser_(f, start_state, std::move(payload), false), order_(order) { | ||
| if (advance) { | ||
|
|
@@ -94,13 +95,13 @@ class ComparableStateTraverser final { | |
| } | ||
|
|
||
| ComparableStateTraverser() = delete; | ||
| ComparableStateTraverser &operator=(ComparableStateTraverser const &) = delete; | ||
| ComparableStateTraverser(const ComparableStateTraverser &that) = delete; | ||
| ComparableStateTraverser& operator=(ComparableStateTraverser const&) = delete; | ||
| ComparableStateTraverser(const ComparableStateTraverser& that) = delete; | ||
|
|
||
| /** | ||
| * Comparison of the state traverser for the purpose of ordering them | ||
| */ | ||
| bool operator<(const ComparableStateTraverser &rhs) const { | ||
| bool operator<(const ComparableStateTraverser& rhs) const { | ||
| int compare = std::memcmp(label_stack_.data(), rhs.label_stack_.data(), | ||
| std::min(label_stack_.size(), rhs.label_stack_.size()) * sizeof(label_t)); | ||
| if (compare != 0) { | ||
|
|
@@ -114,24 +115,24 @@ class ComparableStateTraverser final { | |
| return order_ > rhs.order_; | ||
| } | ||
|
|
||
| bool operator>(const ComparableStateTraverser &rhs) const { return rhs.operator<(*this); } | ||
| bool operator>(const ComparableStateTraverser& rhs) const { return rhs.operator<(*this); } | ||
|
|
||
| bool operator<=(const ComparableStateTraverser &rhs) const { return !operator>(rhs); } | ||
| bool operator<=(const ComparableStateTraverser& rhs) const { return !operator>(rhs); } | ||
|
|
||
| bool operator>=(const ComparableStateTraverser &rhs) const { return !operator<(rhs); } | ||
| bool operator>=(const ComparableStateTraverser& rhs) const { return !operator<(rhs); } | ||
|
|
||
| /** | ||
| * Compare traverser with another one, _ignoring_ the order value | ||
| */ | ||
| bool operator==(const ComparableStateTraverser &rhs) const { | ||
| bool operator==(const ComparableStateTraverser& rhs) const { | ||
| if (label_stack_.size() != rhs.label_stack_.size()) { | ||
| return false; | ||
| } | ||
|
|
||
| return std::memcmp(label_stack_.data(), rhs.label_stack_.data(), label_stack_.size() * sizeof(label_t)) == 0; | ||
| } | ||
|
|
||
| bool operator!=(const ComparableStateTraverser &rhs) const { return !operator==(rhs); } | ||
| bool operator!=(const ComparableStateTraverser& rhs) const { return !operator==(rhs); } | ||
|
|
||
| operator bool() const { return state_traverser_; } | ||
|
|
||
|
|
@@ -162,7 +163,7 @@ class ComparableStateTraverser final { | |
|
|
||
| label_t GetStateLabel() const { return state_traverser_.GetStateLabel(); } | ||
|
|
||
| const std::vector<label_t> &GetStateLabels() const { return label_stack_; } | ||
| [[nodiscard]] const std::vector<label_t>& GetStateLabels() const { return label_stack_; } | ||
|
|
||
| size_t GetOrder() const { return order_; } | ||
|
|
||
|
|
@@ -186,22 +187,22 @@ class ComparableStateTraverser final { | |
| template <class nearInnerTraverserType> | ||
| friend class matching::NearMatching; | ||
|
|
||
| traversal::TraversalState<transition_t> &GetStates() { return state_traverser_.GetStates(); } | ||
| traversal::TraversalState<transition_t>& GetStates() { return state_traverser_.GetStates(); } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "keyvi::dictionary::fsa::traversal::TraversalState" is directly included [misc-include-cleaner] traversal::TraversalState<transition_t>& GetStates() { return state_traverser_.GetStates(); }
^ |
||
|
|
||
| traversal::TraversalPayload<transition_t> &GetTraversalPayload() { return state_traverser_.GetTraversalPayload(); } | ||
| traversal::TraversalPayload<transition_t>& GetTraversalPayload() { return state_traverser_.GetTraversalPayload(); } | ||
|
|
||
| const traversal::TraversalPayload<transition_t> &GetTraversalPayload() const { | ||
| [[nodiscard]] const traversal::TraversalPayload<transition_t>& GetTraversalPayload() const { | ||
| return state_traverser_.GetTraversalPayload(); | ||
| } | ||
| }; | ||
|
|
||
| inline bool CompareWeights(const traversal::TraversalState<traversal::WeightedTransition> &i, | ||
| const traversal::TraversalState<traversal::WeightedTransition> &j) { | ||
| inline bool CompareWeights(const traversal::TraversalState<traversal::WeightedTransition>& i, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "keyvi::dictionary::fsa::traversal::TraversalState" is directly included [misc-include-cleaner] inline bool CompareWeights(const traversal::TraversalState<traversal::WeightedTransition>& i,
^
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "keyvi::dictionary::fsa::traversal::WeightedTransition" is directly included [misc-include-cleaner] keyvi/include/keyvi/dictionary/fsa/comparable_state_traverser.h:33: - #include "keyvi/dictionary/fsa/traverser_types.h"
+ #include "keyvi/dictionary/fsa/traversal/weighted_traversal.h"
+ #include "keyvi/dictionary/fsa/traverser_types.h"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: parameter name 'i' is too short, expected at least 3 characters [readability-identifier-length] inline bool CompareWeights(const traversal::TraversalState<traversal::WeightedTransition>& i,
^
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "keyvi::dictionary::fsa::traversal::WeightedTransition" is directly included [misc-include-cleaner] keyvi/include/keyvi/dictionary/fsa/comparable_state_traverser.h:34: - #include "keyvi/dictionary/fsa/traverser_types.h"
+ #include "keyvi/dictionary/fsa/traversal/weighted_traversal.h"
+ #include "keyvi/dictionary/fsa/traverser_types.h" |
||
| const traversal::TraversalState<traversal::WeightedTransition>& j) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: parameter name 'j' is too short, expected at least 3 characters [readability-identifier-length] const traversal::TraversalState<traversal::WeightedTransition>& j) {
^ |
||
| return i.GetNextInnerWeight() == j.GetNextInnerWeight(); | ||
| } | ||
|
|
||
| template <> | ||
| inline bool ComparableStateTraverser<WeightedStateTraverser>::operator<(const ComparableStateTraverser &rhs) const { | ||
| inline bool ComparableStateTraverser<WeightedStateTraverser>::operator<(const ComparableStateTraverser& rhs) const { | ||
| TRACE("operator< (weighted state specialization)"); | ||
|
|
||
| TRACE("depth %ld %ld", state_traverser_.GetDepth(), rhs.state_traverser_.GetDepth()); | ||
|
|
@@ -231,7 +232,7 @@ inline bool ComparableStateTraverser<WeightedStateTraverser>::operator<(const Co | |
| } | ||
|
|
||
| template <> | ||
| inline bool ComparableStateTraverser<NearStateTraverser>::operator==(const ComparableStateTraverser &rhs) const { | ||
| inline bool ComparableStateTraverser<NearStateTraverser>::operator==(const ComparableStateTraverser& rhs) const { | ||
| if (label_stack_.size() != rhs.label_stack_.size()) { | ||
| return false; | ||
| } | ||
|
|
@@ -244,7 +245,7 @@ inline bool ComparableStateTraverser<NearStateTraverser>::operator==(const Compa | |
| } | ||
|
|
||
| template <> | ||
| inline bool ComparableStateTraverser<NearStateTraverser>::operator<(const ComparableStateTraverser &rhs) const { | ||
| inline bool ComparableStateTraverser<NearStateTraverser>::operator<(const ComparableStateTraverser& rhs) const { | ||
| TRACE("operator< (near state specialization)"); | ||
|
|
||
| if (GetTraversalPayload().exact != rhs.GetTraversalPayload().exact) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,21 +66,21 @@ class LeastRecentlyUsedGenerationsCache final { | |
|
|
||
| ~LeastRecentlyUsedGenerationsCache() { | ||
| delete current_generation_; | ||
| for (MinimizationHash<EntryT> *generation : generations_) { | ||
| for (MinimizationHash<EntryT>* generation : generations_) { | ||
| delete generation; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory] delete generation;
^Additional contextkeyvi/include/keyvi/dictionary/fsa/internal/lru_generation_cache.h:68: variable declared here for (MinimizationHash<EntryT>* generation : generations_) {
^ |
||
| } | ||
| } | ||
|
|
||
| LeastRecentlyUsedGenerationsCache() = delete; | ||
| LeastRecentlyUsedGenerationsCache &operator=(LeastRecentlyUsedGenerationsCache const &) = delete; | ||
| LeastRecentlyUsedGenerationsCache(const LeastRecentlyUsedGenerationsCache &that) = delete; | ||
| LeastRecentlyUsedGenerationsCache& operator=(LeastRecentlyUsedGenerationsCache const&) = delete; | ||
| LeastRecentlyUsedGenerationsCache(const LeastRecentlyUsedGenerationsCache& that) = delete; | ||
|
|
||
| /** Add this object. | ||
| * @param key The key to add | ||
| */ | ||
| void Add(EntryT key) { | ||
| if (current_generation_->Size() >= size_per_generation_) { | ||
| MinimizationHash<EntryT> *newGeneration = nullptr; | ||
| MinimizationHash<EntryT>* newGeneration = nullptr; | ||
| if (generations_.size() + 1 == max_number_of_generations_) { | ||
| // remove(free) the first generation | ||
| newGeneration = generations_[0]; | ||
|
|
@@ -101,7 +101,7 @@ class LeastRecentlyUsedGenerationsCache final { | |
| } | ||
|
|
||
| template <typename EqualityType> | ||
| const EntryT Get(EqualityType &key) { // NOLINT | ||
| const EntryT Get(EqualityType& key) { // NOLINT | ||
| EntryT state = current_generation_->Get(key); | ||
|
|
||
| if (!state.IsEmpty()) { | ||
|
|
@@ -126,7 +126,7 @@ class LeastRecentlyUsedGenerationsCache final { | |
| */ | ||
| void Clear() { | ||
| current_generation_->Clear(); | ||
| for (MinimizationHash<EntryT> *generation : generations_) { | ||
| for (MinimizationHash<EntryT>* generation : generations_) { | ||
| delete generation; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory] delete generation;
^Additional contextkeyvi/include/keyvi/dictionary/fsa/internal/lru_generation_cache.h:128: variable declared here for (MinimizationHash<EntryT>* generation : generations_) {
^ |
||
| } | ||
| generations_.clear(); | ||
|
|
@@ -148,8 +148,8 @@ class LeastRecentlyUsedGenerationsCache final { | |
| private: | ||
| size_t size_per_generation_; | ||
| size_t max_number_of_generations_; | ||
| MinimizationHash<EntryT> *current_generation_; | ||
| std::vector<MinimizationHash<EntryT> *> generations_; | ||
| MinimizationHash<EntryT>* current_generation_; | ||
| std::vector<MinimizationHash<EntryT>*> generations_; | ||
| }; | ||
|
|
||
| } /* namespace internal */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -200,9 +200,7 @@ class SparseArrayPersistence final { | |||||
| TRACE("Wrote Transitions, stream at %d", stream.tellp()); | ||||||
| } | ||||||
|
|
||||||
| size_t GetChunkSizeExternalTransitions() const { | ||||||
| return transitions_extern_->GetChunkSize(); | ||||||
| } | ||||||
| size_t GetChunkSizeExternalTransitions() const { return transitions_extern_->GetChunkSize(); } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: function 'GetChunkSizeExternalTransitions' should be marked [[nodiscard]] [modernize-use-nodiscard]
Suggested change
|
||||||
|
|
||||||
| uint32_t GetVersion() const; | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -55,7 +55,7 @@ class StateTraverser final { | |||||
| this->operator++(0); | ||||||
| } | ||||||
|
|
||||||
| StateTraverser(automata_t f, const uint64_t start_state, traversal::TraversalPayload<TransitionT> &&payload, | ||||||
| StateTraverser(automata_t f, const uint64_t start_state, traversal::TraversalPayload<TransitionT>&& payload, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "uint64_t" is directly included [misc-include-cleaner] keyvi/include/keyvi/dictionary/fsa/state_traverser.h:27: - #include <utility>
+ #include <cstdint>
+ #include <utility>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: parameter name 'f' is too short, expected at least 3 characters [readability-identifier-length] StateTraverser(automata_t f, const uint64_t start_state, traversal::TraversalPayload<TransitionT>&& payload,
^
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: the parameter 'f' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
Suggested change
|
||||||
| const bool advance = true) | ||||||
| : fsa_(f), current_weight_(0), current_label_(0), stack_(std::move(payload)) { | ||||||
| current_state_ = start_state; | ||||||
|
|
@@ -81,10 +81,10 @@ class StateTraverser final { | |||||
| } | ||||||
|
|
||||||
| StateTraverser() = delete; | ||||||
| StateTraverser &operator=(StateTraverser const &) = delete; | ||||||
| StateTraverser(const StateTraverser &that) = delete; | ||||||
| StateTraverser& operator=(StateTraverser const&) = delete; | ||||||
| StateTraverser(const StateTraverser& that) = delete; | ||||||
|
|
||||||
| StateTraverser(StateTraverser &&other) | ||||||
| StateTraverser(StateTraverser&& other) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: move constructors should be marked noexcept [cppcoreguidelines-noexcept-move-operations] keyvi/include/keyvi/dictionary/fsa/state_traverser.h:87: - : fsa_(other.fsa_),
+ noexcept : fsa_(other.fsa_), |
||||||
| : fsa_(other.fsa_), | ||||||
| current_state_(other.current_state_), | ||||||
| current_weight_(other.current_weight_), | ||||||
|
|
@@ -182,13 +182,13 @@ class StateTraverser final { | |||||
|
|
||||||
| template <class innerTraverserType> | ||||||
| friend class ComparableStateTraverser; | ||||||
| const traversal::TraversalStack<TransitionT> &GetStack() const { return stack_; } | ||||||
| const traversal::TraversalStack<TransitionT>& GetStack() const { return stack_; } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: function 'GetStack' should be marked [[nodiscard]] [modernize-use-nodiscard]
Suggested change
|
||||||
|
|
||||||
| traversal::TraversalState<transition_t> &GetStates() { return stack_.GetStates(); } | ||||||
| traversal::TraversalState<transition_t>& GetStates() { return stack_.GetStates(); } | ||||||
|
|
||||||
| traversal::TraversalPayload<TransitionT> &GetTraversalPayload() { return stack_.traversal_stack_payload; } | ||||||
| traversal::TraversalPayload<TransitionT>& GetTraversalPayload() { return stack_.traversal_stack_payload; } | ||||||
|
|
||||||
| const traversal::TraversalPayload<TransitionT> &GetTraversalPayload() const { return stack_.traversal_stack_payload; } | ||||||
| const traversal::TraversalPayload<TransitionT>& GetTraversalPayload() const { return stack_.traversal_stack_payload; } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: function 'GetTraversalPayload' should be marked [[nodiscard]] [modernize-use-nodiscard]
Suggested change
|
||||||
| }; | ||||||
|
|
||||||
| /** | ||||||
|
|
||||||
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.
warning: return type 'const uint64_t' (aka 'const unsigned long') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]