Checks if any calls to posix functions (except posix_openpt) expect negative return values.
These functions return either 0 on success or an errno on failure, which is positive only.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 34027 Build 34026: arc lint + arc unit
Event Timeline
just a few drive-by nits/comments from me. as usual, not super familiar with clang-tidy, so i won't be able to stamp this.
thanks!
clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp | ||
---|---|---|
23 ↗ | (On Diff #205897) | should we also try to catch <= ${negative_value} (or < ${negative_value})? |
29 ↗ | (On Diff #205897) | similarly, should we complain about != ${negative_value}? |
36 ↗ | (On Diff #205897) | nit: please clang-format |
39 ↗ | (On Diff #205897) | would it be helpful to add fixits for simple cases? e.g. we can probably offer to replace posix_whatever() < 0 with posix_whatever() > 0 or != 0 |
clang-tools-extra/test/clang-tidy/android-posix-return.cpp | ||
57 ↗ | (On Diff #205897) | nit: no space in Macro (), please |
I implemented it for Andriod as requested, but it would be completely fine to move it if it is better to place the check at a different location. Where do you suggest we should move this check to? Thanks.
clang-tools-extra/clang-tidy/android/PosixReturnCheck.h | ||
---|---|---|
1 ↗ | (On Diff #205897) | Please use - before *- C++ -* to fill line. See other checks as example. |
clang-tools-extra/docs/ReleaseNotes.rst | ||
205 | Please move into new checks list (in alphabetical order). | |
208 | calls? posix -> POSIX. Please enclose posix_openpt in double back-ticks. | |
clang-tools-extra/docs/clang-tidy/checks/android-posix-return.rst | ||
4 ↗ | (On Diff #205897) | Please make same length as name. |
6 ↗ | (On Diff #205897) | Please make same changes as in Release Notes. |
7 ↗ | (On Diff #205897) | Please enclose 0 and errno into double back-ticks. |
16 ↗ | (On Diff #205897) | Please add colon at end. |
clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp | ||
---|---|---|
23 ↗ | (On Diff #205897) | Sounds good. Will update the patch to catch these two cases as well. |
29 ↗ | (On Diff #205897) | Sounds good. Will update the patch to catch these additional cases as well. |
39 ↗ | (On Diff #205897) | While this fix is handy, I am not sure whether it will be safe enough under all circumstances. For example, is it possible in the code block following the check, the program calls another POSIX function and alter the errno before its value it checked? In that case, maybe the proper fix should be something as follows and fixing it by changing the binary operator may obscure it: int ret = posix_whatever(); |
Sounds good. Thanks for the clarification. The check has been moved to 'bugprone' module.
clang-tools-extra/docs/clang-tidy/checks/android-posix-return.rst | ||
---|---|---|
21 ↗ | (On Diff #206273) | why not if (posix_fadvise() != 0) ? Otherwise it looks like adding a variable is necessary for a fix. |
clang-tools-extra/test/clang-tidy/android-posix-return.cpp | ||
8 ↗ | (On Diff #206273) | typedef decltype(sizeof(char)) size_t; |
67 ↗ | (On Diff #206273) | What about if (posix_fadvise() >= 0) { ... } else { ... } ? |
clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp | ||
---|---|---|
39 ↗ | (On Diff #205897) | After some offline discussion, I agree fixing the simple cases should be fine as programers should verify if the fixes proposed by clang-tidy are correct. |
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp | ||
---|---|---|
50 | The fix-it is triggered when cases like posix_*(...) < 0 are detected (except posix_openpt) , which are currently covered in the unit test. The test runtime will make sure fix-its are applied. | |
clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp | ||
97 | This check only matches calls to the POSIX functions in the global scope, not member functions or functions defined within namespaces. Although in the global scope, this particular case will still pass as there won't be a `<` binary operator for the else branch so no matching will happen. |
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp | ||
---|---|---|
50 | Could you add CHECK-FIXES lines to validate the fixit directly? | |
clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp | ||
97 | Sorry, yes, that's what I meant -- if we warn on < 0, then we should warn on the else branch of >=. I just commented on the wrong test -- sorry about that. |
Update the check to catch redundant code and update the warning message to be more speicific.
clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp | ||
---|---|---|
97 | Thanks for the clarification. Actually >= 0 is always true since the call return 0 on success and a positive value on error, so the else branch will never be reached. I have update my check and the warning message to reflect this. Good catch! |
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp | ||
---|---|---|
62 | 80 columns, please. |
Good improvements!
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp | ||
---|---|---|
22 | Please clang-format (the whole file). | |
57 | "non-negative" "the comparison always evaluates to false because %0 always returns non-negative values" | |
63 | I'd suggest "the comparison always evaluates to true because %0 always returns a non-negative value" |
Totally missed your message yesterday. Sorry about that. I do not have the access so it will be great if you can commit the change. Thanks a lot.
Please clang-format (the whole file).