Skip to content
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

Add more Clang Tidy checks #1654

Merged
merged 11 commits into from Jun 19, 2019

Conversation

@KaustubhShamshery
Copy link
Collaborator

commented May 9, 2019

Description

Enforce checks using clang-tidy

  • Class name = CamelCase
  • Initialize every variable not being initialized by default constructor.
  • bugprone
  • performance

Backward Compatibility

  • This is not a breaking change
  • This is a breaking change

Review Suggestion

Status

Implementation

  • ready for review

Integration Test (Core Team)

  • local machine test
  • small-scale cloud test
@KaustubhShamshery KaustubhShamshery force-pushed the feature/more_clang_tidy_checks branch from de243eb to 9e15f19 May 9, 2019
@codecov-io

This comment has been minimized.

Copy link

commented May 10, 2019

Codecov Report

Merging #1654 into master will decrease coverage by <.01%.
The diff coverage is 43.71%.

@@            Coverage Diff             @@
##           master    #1654      +/-   ##
==========================================
- Coverage   33.57%   33.56%   -0.01%     
==========================================
  Files         270      270              
  Lines       33007    33055      +48     
==========================================
+ Hits        11081    11095      +14     
- Misses      21926    21960      +34
Impacted Files Coverage Δ
src/libConsensus/ConsensusLeader.h 0% <ø> (ø) ⬆️
src/libDirectoryService/DirectoryService.h 0% <ø> (ø) ⬆️
src/libData/BlockData/BlockHeader/DSBlockHeader.h 6.25% <ø> (ø) ⬆️
src/libConsensus/ConsensusCommon.h 25% <ø> (ø) ⬆️
src/libCrypto/Sha2.h 94.11% <ø> (ø) ⬆️
src/libData/DataStructures/CircularArray.h 100% <ø> (ø) ⬆️
src/libCrypto/BIGNUMSerialize.cpp 75% <ø> (ø) ⬆️
src/libCrypto/MultiSig.h 0% <ø> (ø) ⬆️
src/libData/AccountData/AccountStoreSC.h 100% <ø> (ø) ⬆️
src/libNode/Node.h 0% <ø> (ø) ⬆️
... and 82 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2f3ad2...55532e4. Read the comment docs.

@KaustubhShamshery KaustubhShamshery self-assigned this May 10, 2019
@KaustubhShamshery KaustubhShamshery added this to PRs in development in Core via automation May 10, 2019
@KaustubhShamshery KaustubhShamshery force-pushed the feature/more_clang_tidy_checks branch from 0cae3bd to a2ba5a2 May 13, 2019
@KaustubhShamshery KaustubhShamshery requested review from bb111189 and ckyang May 13, 2019
@ansnunez ansnunez moved this from PRs in development to PRs needing review (please help!) in Core May 22, 2019
@@ -1,10 +1,11 @@
---
# TODO: enable more checks, run 'clang-tidy -list-checks -checks="*"' to see more
Checks: '-*,google-global-names-in-headers,modernize-loop-convert,readability-simplify-boolean-expr,readability-inconsistent-declaration-parameter-name'
Checks: '-*,google-global-names-in-headers,modernize-loop-convert,readability-simplify-boolean-expr,readability-inconsistent-declaration-parameter-name,readability-identifier-naming,cppcoreguidelines-pro-type-member-init,bugprone-*, -bugprone-macro-parentheses, -bugprone-lambda-function-name, performance-*'

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 12, 2019

Contributor

For our reference, can you add in the description which check does what?
For example, readability-identifier-naming will check for CamelCase.

Also, just confirming if the hyphen at the start of -bugprone-macro-parentheses and -bugprone-lambda-function-name is actually needed. Couldn't find examples that include the hyphen.

This comment has been minimized.

Copy link
@KaustubhShamshery

KaustubhShamshery Jun 12, 2019

Author Collaborator

There are many checks, it would be hard to document otherwise, https://releases.llvm.org/7.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/list.html

Kindly go through this list.

The hyphen in beginning is to disable those checks

m_tranID.asArray().begin());
m_signature = src.m_signature;
m_coreInfo = src.m_coreInfo;
// Transaction& Transaction::operator=(const Transaction& src) {

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 12, 2019

Contributor

Is this function unused? Maybe we can just remove completely.

This comment has been minimized.

Copy link
@KaustubhShamshery

KaustubhShamshery Jun 12, 2019

Author Collaborator

I think this will be removed when I rebase to current master.

@@ -57,7 +57,7 @@ class ConsensusLeader : public ConsensusCommon {
unsigned int m_numOfSubsets;
// Received commits
std::mutex m_mutex;
std::atomic<unsigned int> m_commitCounter;
std::atomic<unsigned int> m_commitCounter{0};

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 12, 2019

Contributor

Any idea why clang-tidy initializes this as {0} but for other cases it's just {}?

This comment has been minimized.

Copy link
@KaustubhShamshery

KaustubhShamshery Jun 12, 2019

Author Collaborator

manual error, will fix

: m_tranID(src.m_tranID),
m_coreInfo(src.m_coreInfo),
m_signature(src.m_signature) {}
// Transaction::Transaction(const Transaction& src)

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 12, 2019

Contributor

If unused, can just remove completely?

bool m_fallbackTimerLaunched = false;
bool m_fallbackStarted;
bool m_fallbackStarted{};

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 12, 2019

Contributor

What does {} for bool default to?
And should the line immediately above this be {false}?
Curly brace-style initialization takes some getting used to.

This comment has been minimized.

Copy link
@KaustubhShamshery

KaustubhShamshery Jun 12, 2019

Author Collaborator

defaults to false, in present way , it is actually arbitrary

@@ -59,7 +59,7 @@ class Logger {
std::string m_fileName;
std::ofstream m_logFile;
unsigned int m_seqNum;

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 12, 2019

Contributor

It seems like clang-tidy doesn't necessarily complain, since native-type members like this remain unmodified.

This comment has been minimized.

Copy link
@KaustubhShamshery

KaustubhShamshery Jun 12, 2019

Author Collaborator

Initialize every variable not being initialized by default constructor

@@ -33,7 +33,7 @@ struct CommitSecret : public Serializable {
std::shared_ptr<BIGNUM> m_s;

/// Flag to indicate if parameters have been initialized.
bool m_initialized;
bool m_initialized{};

This comment has been minimized.

Copy link
@ShengguangXiao

ShengguangXiao Jun 13, 2019

Contributor

Why don't initialize it to false?

@@ -407,7 +409,7 @@ ConsensusBackup::ConsensusBackup(uint32_t consensus_id, uint64_t block_number,
: ConsensusCommon(consensus_id, block_number, block_hash, node_id, privkey,
committee, class_byte, ins_byte),
m_leaderID(leader_id),
m_msgContentValidator(msg_validator) {
m_msgContentValidator(std::move(msg_validator)) {

This comment has been minimized.

Copy link
@ckyang

ckyang Jun 13, 2019

Contributor

Maybe can remove std::

m_nodeCommitFailureHandlerFunc = nodeCommitFailureHandlerFunc;
m_shardCommitFailureHandlerFunc = shardCommitFailureHandlerFunc;
m_nodeCommitFailureHandlerFunc = std::move(nodeCommitFailureHandlerFunc);
m_shardCommitFailureHandlerFunc = std::move(shardCommitFailureHandlerFunc);

This comment has been minimized.

Copy link
@ckyang

ckyang Jun 13, 2019

Contributor

Maybe can remove std::

@@ -69,7 +69,7 @@ class Transaction : public SerializableDataBlock {
Transaction();

/// Copy constructor.
Transaction(const Transaction& src);
// Transaction(const Transaction& src);

This comment has been minimized.

Copy link
@ckyang

ckyang Jun 13, 2019

Contributor

Please remove unnecessary code

@@ -190,7 +190,7 @@ class Transaction : public SerializableDataBlock {
bool operator>(const Transaction& tran) const;

/// Assignment operator.
Transaction& operator=(const Transaction& src);
// Transaction& operator=(const Transaction& src);

This comment has been minimized.

Copy link
@ckyang

ckyang Jun 13, 2019

Contributor

Please remove unnecessary code

@@ -745,9 +746,9 @@ void P2PComm::StartMessagePump(uint32_t listen_port_host,
};
DetachedFunction(1, funcCheckSendQueue);

m_dispatcher = dispatcher;
m_dispatcher = std::move(dispatcher);

This comment has been minimized.

Copy link
@ckyang

ckyang Jun 13, 2019

Contributor

Can remove std:: here

@@ -71,8 +73,8 @@ void Scheduler::ScheduleAt(std::function<void(void)> f,

void Scheduler::ScheduleAfter(std::function<void(void)> f,
int64_t deltaMilliSeconds) {
ScheduleAt(
f, chrono::system_clock::now() + chrono::milliseconds(deltaMilliSeconds));
ScheduleAt(std::move(f), chrono::system_clock::now() +

This comment has been minimized.

Copy link
@ckyang

ckyang Jun 13, 2019

Contributor

Can remove std:: here

@KaustubhShamshery KaustubhShamshery force-pushed the feature/more_clang_tidy_checks branch from ad12f61 to a815b2c Jun 18, 2019
Core automation moved this from PRs needing review (please help!) to PRs approved - ready to merge! Jun 18, 2019
@ckyang
ckyang approved these changes Jun 19, 2019
@KaustubhShamshery KaustubhShamshery marked this pull request as ready for review Jun 19, 2019
@ansnunez ansnunez merged commit 9b62301 into master Jun 19, 2019
2 checks passed
2 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
Core automation moved this from PRs approved - ready to merge! to PRs done (merged) Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.