-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: add clang-tidy for pika #1515
Conversation
There are several conflicting files left that need to be manually modified. See run_clang_tidy.py.And disabled some clang-tidy checks, eg, use int64_t instead of long or long long, variable naming. Signed-off-by: Hao Lee <1838249551@qq.com>
Signed-off-by: Hao Lee <1838249551@qq.com>
Signed-off-by: Hao Lee <1838249551@qq.com>
Signed-off-by: Hao Lee <1838249551@qq.com>
@@ -14,15 +18,15 @@ | |||
|
|||
class Context { | |||
public: | |||
Context(const std::string path); | |||
Context(std::string path); |
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.
这里应该是建议用std::move吧。
.github/workflows/pika.yml
Outdated
@@ -30,7 +30,7 @@ jobs: | |||
- name: install Deps | |||
if: ${{ steps.cache.output.cache-hit != 'true' }} | |||
run: | | |||
sudo apt install autoconf libprotobuf-dev protobuf-compiler -y | |||
sudo apt install autoconf libprotobuf-dev protobuf-compiler clang-tidy -y |
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.
clang-tidy 建议表明下版本。因为clang-tidy-14 和 clang-tidy-16是存在差异的。
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.
后面cmake表明的是12,建议一致
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.
你的意思是,这里直接把 clang-tidy 改为 clang-tidy-14 么?
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.
如果根据cmakelists.txt中的改动,应该改成clang-tidy-12
find_program(CLANG_TIDY_BIN
NAMES clang-tidy clang-tidy-12
HINTS ${CLANG_SEARCH_PATH})
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.
@ForestLH 你看着改下吧
format if/while
@@ -118,7 +118,7 @@ std::vector<ServerThread::ConnInfo> DispatchThread::conns_info() const { | |||
std::shared_ptr<NetConn> DispatchThread::MoveConnOut(int fd) { | |||
for (int i = 0; i < work_num_; ++i) { | |||
std::shared_ptr<NetConn> conn = worker_thread_[i]->MoveConnOut(fd); | |||
if (conn != nullptr) { | |||
if (conn) { | |||
return conn; | |||
} | |||
} |
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.
Based on the code patch, some suggestions for improvement are:
- Avoid magic numbers (e.g., 0, nullptr) and use proper constants or enums instead.
- Consider using std::unique_ptr for managing memory to make the code more robust and less error-prone.
- Use const references whenever possible to avoid unnecessary copies.
- Add comments/documentation to clarify the purpose of methods and variables.
- Consider adding exception handling to handle errors more gracefully.
As for possible bugs, it's hard to tell without understanding the context and requirements of the code properly. The code seems fine at first sight, but further testing may be necessary to detect possible issues.
char addressBuffer[INET_ADDRSTRLEN]; | ||
inet_ntop(AF_INET, tmpAddrPtr, addressBuffer, INET_ADDRSTRLEN); | ||
if (std::string(ifa->ifa_name) == network_interface) { | ||
host = addressBuffer; | ||
break; | ||
} | ||
} else if (ifa->ifa_addr->sa_family == AF_INET6) { // Check it is a valid IPv6 address | ||
tmpAddrPtr = &((struct sockaddr_in6*)ifa->ifa_addr)->sin6_addr; | ||
tmpAddrPtr = &(reinterpret_cast<struct sockaddr_in6*>(ifa->ifa_addr))->sin6_addr; | ||
char addressBuffer[INET6_ADDRSTRLEN]; | ||
inet_ntop(AF_INET6, tmpAddrPtr, addressBuffer, INET6_ADDRSTRLEN); | ||
if (std::string(ifa->ifa_name) == network_interface) { |
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.
Overall, the code seems to be well-written, but here are a few improvements:
-
Add comments: The code can benefit from comments explaining the purpose of each function and block of code.
-
Use nullptr instead of NULL: The code uses NULL in some places, which is an older way of representing a null pointer. C++11 introduced nullptr, which should be preferred.
-
Use std::unique_ptr instead of raw pointers: The code uses raw pointers extensively, which can lead to memory leaks and undefined behavior. Using smart pointers (
std::unique_ptr
, in this case) can automatically handle memory allocations and deallocations. -
Simplify if statements: Some of the if statements could be simplified by using boolean expressions instead of nested if blocks.
-
Avoid unnecessary typecasting: In a few places, the code casts variables from one type to another unnecessarily. This can make the code harder to read and potentially introduce errors.
-
Use const-correctness: Where possible, functions and variables should be declared as const to prevent accidental modification.
-
Be consistent with braces usage: The code uses both one-line and multi-line if statements without braces, which can lead to confusion and bugs. It's best to always use braces, even for one-line statements.
-
Improve variable naming: Some variable names (e.g.
tmpAddrPtr
) are not very descriptive. Better variable names can make the code easier to understand. -
Check for error conditions: Some functions fail silently without checking for errors. It's best to explicitly check for errors and handle them appropriately. For example, the
inet_ntop
function can fail, so its return value should be checked.
@@ -149,7 +151,7 @@ int main(int argc, char* argv[]) { | |||
} | |||
} | |||
|
|||
if (path_opt == false) { | |||
if (!path_opt) { | |||
fprintf(stderr, "Please specify the conf file path\n"); | |||
usage(); | |||
exit(-1); |
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.
The changes to the code involve replacing signal.h
with csignal
and adding braces to the if
statement in the daemonize()
function. Additionally, int
casts are added for getpid()
in the create_pid_file()
function.
In terms of improving the code, it would be useful to add error handling for when the pid file cannot be created. The code could also benefit from refactoring into functions with clear responsibilities instead of having everything in the main()
function. Furthermore, command-line arguments and options could be parsed using a third-party library like boost::program_options
for more robustness.
fix ci failure
Signed-off-by: Hao Lee <1838249551@qq.com>
Signed-off-by: Hao Lee <1838249551@qq.com>
remove redundant extern statement and use default Signed-off-by: Hao Lee <1838249551@qq.com>
fix:build ci error and format codes
src/net/include/thread_pool.h
Outdated
/* | ||
* No allowed copy and copy assign | ||
*/ | ||
ThreadPool(const ThreadPool&); |
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.
这部分要删掉
这个noncopyable的拷贝构造函数和=函数,应该设置为public吧 private:
noncopyable(const noncopyable&) = delete;
void operator=(const noncopyable&) = delete; |
remove redundant extern g_pika_conf statement and correct if statement judgement instead of assignment Signed-off-by: Hao Lee <1838249551@qq.com>
style:format codes
你看下 https://www.boost.org/doc/libs/1_64_0/boost/core/noncopyable.hpp |
merge upstream unstable and format codes
a huge contribution! |
haha. I have worked on this pr for this whole week. |
@ForestLH If you have questions about noncopyable, you can view the template in muduo(base/noncopyable) or the above boost. If you use private funcs for copy func or public funcs for deleted copy func, you both get the right answer. |
format codes again
LOG(WARNING) << "Binlog reader init failed"; | ||
return; | ||
} | ||
|
||
BinlogItem item; | ||
BinlogOffset offset; | ||
while (1) { | ||
while (true) { | ||
std::string binlog; | ||
Status s = binlog_reader.Get(&binlog, &(offset.filenum), &(offset.offset)); | ||
if (s.IsEndFile()) { |
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.
Overall, the code patch looks fine. Here are some suggestions:
- In line 7-8, you can add a blank line between #include statements for clarity and better visual separation.
- In line 13, instead of using "using" statement, explicitly qualify the namespace for the "Status", like "pstd::Status".
- In lines 15-16, use move semantics to initialize the member variables table_name_ and log_path_, since we don't need their values after assigning them to the class members anymore.
- In line 44, the default destructor is used, which is fine. You can remove that line completely.
- In line 52, !=0 is not necessary. You can simply use the boolean value returned by RenameFile function.
- In lines 75-76, use std::make_unique instead of raw pointer new expression to allocate memory with automatic resource management.
- In lines 97-98, use nullptr instead of NULL, which is a macro from C.
- In lines 134-135, use the boolean value returned by GetChildren function instead of comparing it against zero.
These are minor suggestions, and the code patch is perfectly functional as it stands.
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.
LGTM
* feature:add clang-tidy for pika There are several conflicting files left that need to be manually modified. See run_clang_tidy.py.And disabled some clang-tidy checks, eg, use int64_t instead of long or long long, variable naming. Signed-off-by: Hao Lee <1838249551@qq.com> * feature:WIP modified clang-tidy file Signed-off-by: Hao Lee <1838249551@qq.com> * feature:disable compile command clang-tidy check Signed-off-by: Hao Lee <1838249551@qq.com> * style:Make the code standard according to clang-tidy Signed-off-by: Hao Lee <1838249551@qq.com> * delete double std::move * auto* -> auto * using noncopyable * add space for override{} * add spaces * delete != 0 * format if-return * clear inheritage * format codes * format codes * delete == 0 * fix:fix build error Signed-off-by: Hao Lee <1838249551@qq.com> * style:disable check bool implicit convert in clang-tidy Signed-off-by: Hao Lee <1838249551@qq.com> * style:update code style Signed-off-by: Hao Lee <1838249551@qq.com> * delete '!= nullptr' * format if/while * fix ci failure * fix:build ci error Signed-off-by: Hao Lee <1838249551@qq.com> * style:The judgment statement should not be an assignment Signed-off-by: Hao Lee <1838249551@qq.com> * style:format codes remove redundant extern statement and use default Signed-off-by: Hao Lee <1838249551@qq.com> * style:format codes remove redundant extern g_pika_conf statement and correct if statement judgement instead of assignment Signed-off-by: Hao Lee <1838249551@qq.com> * format codes * format codes --------- Signed-off-by: Hao Lee <1838249551@qq.com> Co-authored-by: alexstocks <alexstocks@foxmail.com> Co-authored-by: Xin.Zh <dragoncharlie@foxmail.com>
* feature:add clang-tidy for pika There are several conflicting files left that need to be manually modified. See run_clang_tidy.py.And disabled some clang-tidy checks, eg, use int64_t instead of long or long long, variable naming. Signed-off-by: Hao Lee <1838249551@qq.com> * feature:WIP modified clang-tidy file Signed-off-by: Hao Lee <1838249551@qq.com> * feature:disable compile command clang-tidy check Signed-off-by: Hao Lee <1838249551@qq.com> * style:Make the code standard according to clang-tidy Signed-off-by: Hao Lee <1838249551@qq.com> * delete double std::move * auto* -> auto * using noncopyable * add space for override{} * add spaces * delete != 0 * format if-return * clear inheritage * format codes * format codes * delete == 0 * fix:fix build error Signed-off-by: Hao Lee <1838249551@qq.com> * style:disable check bool implicit convert in clang-tidy Signed-off-by: Hao Lee <1838249551@qq.com> * style:update code style Signed-off-by: Hao Lee <1838249551@qq.com> * delete '!= nullptr' * format if/while * fix ci failure * fix:build ci error Signed-off-by: Hao Lee <1838249551@qq.com> * style:The judgment statement should not be an assignment Signed-off-by: Hao Lee <1838249551@qq.com> * style:format codes remove redundant extern statement and use default Signed-off-by: Hao Lee <1838249551@qq.com> * style:format codes remove redundant extern g_pika_conf statement and correct if statement judgement instead of assignment Signed-off-by: Hao Lee <1838249551@qq.com> * format codes * format codes --------- Signed-off-by: Hao Lee <1838249551@qq.com> Co-authored-by: alexstocks <alexstocks@foxmail.com> Co-authored-by: Xin.Zh <dragoncharlie@foxmail.com>
fix #1303
WIP
打算分为3个部分,
将long或者是long long换成int64_t,还有对应的unsigned long long
使用方法,进入输出编译结果的文件夹中,例如output
cd output
make clang-tidy
最后,pika仍然有部分会编译失败的代码,例如src/storage/benchmark,所以我在run_clang_tidy.py里面留了一个TODO,目前手动排除了对这些文件的检查