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/09/03 05:39:08 UTC
[GitHub] [incubator-nuttx] easonxiang opened a new pull request, #6995: mm: Check the function result with suitable macro.
easonxiang opened a new pull request, #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995
The return value of function mm_takesemaphore will never below
than zero, DEBUGVERIFY make no effect to check it, use DEBUGASSERT
instead.
Signed-off-by: xiangdong6 <xi...@xiaomi.com>
## Summary
## Impact
None
## Testing
passed.
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962153628
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
Both define are not empty when CONFIG_DEBUG_ASSERTIONS isn't defined
`# define DEBUGASSERT(f) ((void)(1 || (f)))`
`# define DEBUGVERIFY(f) ((void)(f))`
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962142987
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
but DEBUGASSERT become empty when CONFIG_DEBUG_ASSERTIONS isn't defined
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962171656
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
or use ? : like this:
```
DEBUGVERIFY(mm_takesemaphore(heap) ? 0 : -EINVAL);
```
or add DEBUGVERIFYBOOL
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962438813
##########
mm/mm_heap/mm_malloc.c:
##########
@@ -104,6 +104,7 @@ void mm_dump_handler(FAR struct tcb_s *tcb, FAR void *arg)
FAR void *mm_malloc(FAR struct mm_heap_s *heap, size_t size)
{
+ bool val;
Review Comment:
done.
##########
mm/mm_heap/mm_realloc.c:
##########
@@ -64,6 +64,7 @@
FAR void *mm_realloc(FAR struct mm_heap_s *heap, FAR void *oldmem,
size_t size)
{
+ bool ret;
Review Comment:
done.
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962438309
##########
mm/mm_heap/mm_extend.c:
##########
@@ -52,6 +52,7 @@
void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
int region)
{
+ bool ret;
Review Comment:
done.
--
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] gustavonihei commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962203720
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
> Another method:
>
> what's your opinion? @xiaoxiang781216 @pkarashchenko
This and add `unused_data` to `ret` definition.
https://github.com/apache/incubator-nuttx/blob/1b9c013dade651d6998c5b9463e4788282b12e4f/include/nuttx/compiler.h#L228
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962438956
##########
mm/mm_heap/mm_initialize.c:
##########
@@ -58,6 +58,7 @@
void mm_addregion(FAR struct mm_heap_s *heap, FAR void *heapstart,
size_t heapsize)
{
+ bool ret;
Review Comment:
done.
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962769645
##########
mm/mm_heap/mm_memalign.c:
##########
@@ -51,6 +51,7 @@
FAR void *mm_memalign(FAR struct mm_heap_s *heap, size_t alignment,
size_t size)
{
+ bool ret;
Review Comment:
done.
--
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] easonxiang commented on pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#issuecomment-1236618215
Push to github timeout, it's now up to date.
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962170776
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
Could be, but you need handle the unused local variable warning.
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962153956
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
Yes, not really but similar to empty, since this first one will skip mm_takesemaphore when CONFIG_DEBUG_ASSERTIONS isn't defined.
--
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] gustavonihei commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962203720
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
> Another method:
>
> what's your opinion? @xiaoxiang781216 @pkarashchenko
This and declare `ret` with the `unused_data` attribute.
https://github.com/apache/incubator-nuttx/blob/1b9c013dade651d6998c5b9463e4788282b12e4f/include/nuttx/compiler.h#L228
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962230397
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
The `ret` is referenced wether CONFIG_DEBUG_ASSERTIONS defined or not, so, unused_data is not required. I have build with enable and disable CONFIG_DEBUG_ASSERTIONS, there is no addition warnings.
```
#define ASSERT(f) do { if (!(f)) PANIC(); } while (0)
# define DEBUGASSERT(f) ((void)(1 || (f)))
```
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962161077
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
could be
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962153956
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
Yes, but the result is no-op, since this first one will skip mm_takesemaphore when CONFIG_DEBUG_ASSERTIONS isn't defined.
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962154462
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
We can't use `DEBUGASSERT`. We need to evaluate some other way
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962406496
##########
mm/mm_heap/mm_extend.c:
##########
@@ -52,6 +52,7 @@
void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
int region)
{
+ bool ret;
Review Comment:
move after line 60
##########
mm/mm_heap/mm_malloc.c:
##########
@@ -104,6 +104,7 @@ void mm_dump_handler(FAR struct tcb_s *tcb, FAR void *arg)
FAR void *mm_malloc(FAR struct mm_heap_s *heap, size_t size)
{
+ bool val;
Review Comment:
ditto
##########
mm/mm_heap/mm_realloc.c:
##########
@@ -64,6 +64,7 @@
FAR void *mm_realloc(FAR struct mm_heap_s *heap, FAR void *oldmem,
size_t size)
{
+ bool ret;
Review Comment:
ditto
##########
mm/mm_heap/mm_initialize.c:
##########
@@ -58,6 +58,7 @@
void mm_addregion(FAR struct mm_heap_s *heap, FAR void *heapstart,
size_t heapsize)
{
+ bool ret;
Review Comment:
move after line 63
##########
mm/mm_heap/mm_memalign.c:
##########
@@ -51,6 +51,7 @@
FAR void *mm_memalign(FAR struct mm_heap_s *heap, size_t alignment,
size_t size)
{
+ bool ret;
Review Comment:
ditto
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962570805
##########
mm/mm_heap/mm_memalign.c:
##########
@@ -51,6 +51,7 @@
FAR void *mm_memalign(FAR struct mm_heap_s *heap, size_t alignment,
size_t size)
{
+ bool ret;
Review Comment:
no change?
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962159273
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
Got it.
How about change the mm_takesemaphore return value to 0 if success, and negative if fails.
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962167636
##########
mm/mm_heap/mm_extend.c:
##########
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- DEBUGVERIFY(mm_takesemaphore(heap));
+ DEBUGASSERT(mm_takesemaphore(heap));
Review Comment:
Another method:
```suggestion
ret = mm_takesemaphore(heap)
DEBUGASSERT(ret);
```
what's your opinion? @xiaoxiang781216 @pkarashchenko
--
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] easonxiang commented on a diff in pull request #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
easonxiang commented on code in PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#discussion_r962438885
##########
mm/mm_heap/mm_memalign.c:
##########
@@ -51,6 +51,7 @@
FAR void *mm_memalign(FAR struct mm_heap_s *heap, size_t alignment,
size_t size)
{
+ bool ret;
Review Comment:
done.
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995#issuecomment-1236608436
@easonxiang do you forget to push the latest change?
--
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 #6995: mm: Check the function result with suitable macro.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6995:
URL: https://github.com/apache/incubator-nuttx/pull/6995
--
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