You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/10/20 16:16:49 UTC
[GitHub] [incubator-nuttx] GUIDINGLI opened a new pull request, #7372: use DEBUGASSERT instead of unnecessary check
GUIDINGLI opened a new pull request, #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372
## Summary
use DEBUGASSERT instead of unnecessary check
## Impact
sem
mqueue
signal
## Testing
VELA
--
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] [incubator-nuttx] GUIDINGLI commented on pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1301606128
@davids5 @pkarashchenko
Others always do like this, why this PR can't pass ?
like:
https://github.com/apache/incubator-nuttx/blob/8be4bca3ebdd7f2b3e6f35bbdae649c2baef1bee/sched/semaphore/sem_clockwait.c#L106
https://github.com/apache/incubator-nuttx/blob/8be4bca3ebdd7f2b3e6f35bbdae649c2baef1bee/sched/mqueue/mq_sndinternal.c#L75
https://github.com/apache/incubator-nuttx/blob/8be4bca3ebdd7f2b3e6f35bbdae649c2baef1bee/sched/mqueue/mq_rcvinternal.c#L71
This also for the speed up.
--
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] [incubator-nuttx] xiaoxiang781216 merged pull request #7372: move param check to sem_xx level
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372
--
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] [incubator-nuttx] xiaoxiang781216 commented on pull request #7372: move param check to sem_xx level
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1308990836
@davids5 @pkarashchenko do you have.more concern with the new approach?
--
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] [incubator-nuttx] davids5 commented on pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1300884246
@GUIDINGLI Same concern perimeter checking is not a debug activity, it is API calling validation. Why remove it?
--
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] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#discussion_r1012374168
##########
sched/semaphore/sem_wait.c:
##########
@@ -72,10 +72,17 @@ int nxsem_wait(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
irqstate_t flags;
- int ret = -EINVAL;
+ int ret;
/* This API should not be called from interrupt handlers & idleloop */
+#ifdef CONFIG_DEBUG_FEATURES
+ if (sem == NULL)
+ {
+ return -EINVAL;
+ }
+#endif
Review Comment:
https://pubs.opengroup.org/onlinepubs/7908799/xsh/sem_wait.html states:
```
[EINVAL]
The sem argument does not refer to a valid semaphore.
```
So that is not a debug feature
--
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] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7372: move param check to sem_xx level
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#discussion_r1018398697
##########
sched/semaphore/sem_clockwait.c:
##########
@@ -268,6 +260,16 @@ int sem_clockwait(FAR sem_t *sem, clockid_t clockid,
{
int ret;
+ /* Verify the input parameters and, in case of an error, set
+ * errno appropriately.
+ */
+
+ if (abstime == NULL || sem == NULL)
Review Comment:
```suggestion
if (sem == NULL || abstime == NULL)
```
just from common sense perspective
--
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] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7372: use DEBUGASSERT instead of unnecessary check
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#discussion_r1000940325
##########
libs/libc/semaphore/sem_init.c:
##########
@@ -67,31 +68,28 @@ int nxsem_init(FAR sem_t *sem, int pshared, unsigned int value)
* range.
*/
- if (sem != NULL && value <= SEM_VALUE_MAX)
- {
- /* Initialize the semaphore count */
+ DEBUGASSERT(sem != NULL && value <= SEM_VALUE_MAX);
Review Comment:
That is not quite good from application perspective
--
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] [incubator-nuttx] davids5 commented on pull request #7372: use DEBUGASSERT instead of unnecessary check
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1286861147
@GUIDINGLI
Value checking is not just a debug activity.
Why do you deem these to be unnecessary?
--
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] [incubator-nuttx] GUIDINGLI commented on pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1300798794
@davids5 @pkarashchenko
please review again
--
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] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7372: use DEBUGASSERT instead of unnecessary check
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#discussion_r1000941983
##########
libs/libc/semaphore/sem_init.c:
##########
@@ -67,31 +68,28 @@ int nxsem_init(FAR sem_t *sem, int pshared, unsigned int value)
* range.
*/
- if (sem != NULL && value <= SEM_VALUE_MAX)
- {
- /* Initialize the semaphore count */
+ DEBUGASSERT(sem != NULL && value <= SEM_VALUE_MAX);
Review Comment:
I mean the check either should be added to `sem_` layer or kept here
--
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] [incubator-nuttx] GUIDINGLI commented on pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1301606964
We always need user open CONFIG_DEBUG_FEATURES, if he cares the returns.
Or it can speed up !
--
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] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#discussion_r1012483761
##########
libs/libc/semaphore/sem_init.c:
##########
@@ -24,6 +24,7 @@
#include <nuttx/config.h>
+#include <assert.h>
Review Comment:
remove
--
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] [incubator-nuttx] xiaoxiang781216 commented on pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1303889762
@pkarashchenko @davids5 If we want to run fast than threadx, the argument check in the key functions(e.g. sem_xxx) need to remove at least in the production code. It's also a practice in the current code to guard the argument check by CONFIG_DEBUG_FEATURES. What do you think?
--
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] [incubator-nuttx] GUIDINGLI commented on pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1305798681
@davids5 @pkarashchenko
I used another better way to improve the speed, please review, thanks
--
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] [incubator-nuttx] pkarashchenko commented on pull request #7372: use CONFIG_DEBUG_FEATURES to cover param check
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #7372:
URL: https://github.com/apache/incubator-nuttx/pull/7372#issuecomment-1305322441
@xiaoxiang781216 @GUIDINGLI I think we need to define what has more priority, standard compliance or speed. Till now I was (and I am) assuming that standard compliance if the top. If some speed optimizations are needed, then those should be selected via a separate config option as @davids5 says. I do not see any other option here
--
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