You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "pkarashchenko (via GitHub)" <gi...@apache.org> on 2023/01/25 23:38:23 UTC
[GitHub] [nuttx] pkarashchenko opened a new pull request, #8242: nuttx: apply misc fixes in code noticed during code review
pkarashchenko opened a new pull request, #8242:
URL: https://github.com/apache/nuttx/pull/8242
## Summary
Apply misc fixes in code noticed during code review
## Impact
N/A
## Testing
Pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#discussion_r1088323369
##########
fs/shm/shm_unlink.c:
##########
@@ -155,8 +155,8 @@ int shm_unlink(FAR const char *name)
if (ret < 0)
{
set_errno(-ret);
- return -1;
+ return ERROR;
}
- return 0;
+ return ret;
Review Comment:
Here I will fix. My bad.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on code in PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#discussion_r1088299172
##########
fs/shm/shm_open.c:
##########
@@ -165,7 +166,7 @@ int shm_open(FAR const char *name, int oflag, mode_t mode)
if (ret < 0)
{
set_errno(-ret);
- return -1;
+ return ERROR;
Review Comment:
This is matter of opinion of course,but I find these changes confusing. Posix spec says that the function is supposed to return -1. Not "ERROR", which is nuttx internal define.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#discussion_r1088327877
##########
fs/shm/shm_unlink.c:
##########
@@ -155,8 +155,8 @@ int shm_unlink(FAR const char *name)
if (ret < 0)
{
set_errno(-ret);
- return -1;
+ return ERROR;
}
- return 0;
+ return ret;
Review Comment:
I mean it should be `OK` and not `ret`. Just to filter out positive values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on code in PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#discussion_r1088300875
##########
fs/shm/shm_unlink.c:
##########
@@ -155,8 +155,8 @@ int shm_unlink(FAR const char *name)
if (ret < 0)
{
set_errno(-ret);
- return -1;
+ return ERROR;
Review Comment:
Same here, sorry to comment an already merged PR, but shm_unlink returns only 0 or -1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#issuecomment-1405605447
I don't think that posix API should return nuttx defines such as ERROR or OK.Of course no harm done here, since ERROR happens to be -1. It is however just making reading code difficult.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#issuecomment-1405629550
> I don't think that posix API should return nuttx defines such as ERROR or OK.Of course no harm done here, since ERROR happens to be -1. It is however just making reading code difficult.
I just updated to use the same style as in most of the other places (like `sem_post` or `sem_wait` and many others).
Do you recommend replacing all `ERROR` with `-1` in all POSIX API return points?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #8242:
URL: https://github.com/apache/nuttx/pull/8242
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#issuecomment-1405641459
> > I don't think that posix API should return nuttx defines such as ERROR or OK.Of course no harm done here, since ERROR happens to be -1. It is however just making reading code difficult.
>
> I just updated to use the same style as in most of the other places (like `sem_post` or `sem_wait` and many others).
> Do you recommend replacing all `ERROR` with `-1` in all POSIX API return points?
My recommendation is to always explicitly return what the standard says from standard functions. That is, if the standard tells that the function should return 5, one should write "return 5;" in the code and not hide it behind a macro.
But, the functionality is correct I suppose,.so no point in changing back and forth. It only stuck my eye...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#discussion_r1088399683
##########
fs/shm/shm_unlink.c:
##########
@@ -155,8 +155,8 @@ int shm_unlink(FAR const char *name)
if (ret < 0)
{
set_errno(-ret);
- return -1;
+ return ERROR;
}
- return 0;
+ return ret;
Review Comment:
I just looked into code again and all is fine. I put `ret` here just because `file_shm_unlink` has `DEBUGASSERT(ret == OK);`. Let's keep it as is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8242: nuttx: apply misc fixes in code noticed during code review
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on code in PR #8242:
URL: https://github.com/apache/nuttx/pull/8242#discussion_r1088342199
##########
fs/shm/shm_unlink.c:
##########
@@ -155,8 +155,8 @@ int shm_unlink(FAR const char *name)
if (ret < 0)
{
set_errno(-ret);
- return -1;
+ return ERROR;
}
- return 0;
+ return ret;
Review Comment:
Ok, OK is 0 so the functionality is according to the spec. Maybe the new style looks better to other people :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org