You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2023/06/28 16:41:22 UTC
[GitHub] [arrow] felipecrv opened a new issue, #36360: [C++] Reduce number of internal APIs that define default_memory_pool() as default argument value
felipecrv opened a new issue, #36360:
URL: https://github.com/apache/arrow/issues/36360
### Describe the enhancement requested
Default argument values are convenient, but create implicit uses that are hard to see by simply scanning code at function callsites.
Function that accept a `MemoryPool *` should (ideally) only call functions that perform allocations with that memory pool. Due to many functions being declared with `MemoryPool *pool = default_memory_pool()`, it's very easy to accidentally use the default memory pool when you should be passing down the memory pool you received.
Fixing this issue has two steps:
1) Changing internal functions to not define `default_memory_pool()` as default argument and changing the callsites to pass a memory pool they receive from callers of explicitly pass the `default_memory_pool()`.
2) Change declarations of functions that call `default_memory_pool()` to take a `MemoryPool *` parameter instead. Then go to step 1 again.
Related issues:
- #36293
- #34025
### Component(s)
C++
--
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: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on issue #36360: [C++] Reduce number of internal APIs that define default_memory_pool() as default argument value
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on issue #36360:
URL: https://github.com/apache/arrow/issues/36360#issuecomment-1611758000
@wjones127 @pitrou @marin-ma
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Reduce number of internal APIs that define default_memory_pool() as default argument value [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on issue #36360:
URL: https://github.com/apache/arrow/issues/36360#issuecomment-1812780143
@ShaiviAgarwal2 please stop sending ChatGPT-generated comments to the issue tracker.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Reduce number of internal APIs that define default_memory_pool() as default argument value [arrow]
Posted by "ShaiviAgarwal2 (via GitHub)" <gi...@apache.org>.
ShaiviAgarwal2 commented on issue #36360:
URL: https://github.com/apache/arrow/issues/36360#issuecomment-1811694640
@felipecrv Could you please check whether I'm understanding it correctly or not!!
To enhance the code, we need to modify the internal functions to not define `default_memory_pool()` as a default argument and change the callsites to pass a memory pool they receive from callers or explicitly pass the `default_memory_pool()`. Then we need to change the declarations of functions that call `default_memory_pool()` to take a `MemoryPool * `parameter instead. Finally, we should always pass `default_memory_pool()` explicitly in the Arrow code even when a default value is defined in the declaration.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Reduce number of internal APIs that define default_memory_pool() as default argument value [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #36360:
URL: https://github.com/apache/arrow/issues/36360#issuecomment-1812787802
I've marked the message as spam.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Reduce number of internal APIs that define default_memory_pool() as default argument value [arrow]
Posted by "ShaiviAgarwal2 (via GitHub)" <gi...@apache.org>.
ShaiviAgarwal2 commented on issue #36360:
URL: https://github.com/apache/arrow/issues/36360#issuecomment-1809944361
@felipecrv
The enhancement requested is to improve the code by making it easier to see when a function is using the default memory pool instead of the intended memory pool. This is done by changing the internal functions to not define `default_memory_pool()` as the default argument and changing the callsites to explicitly pass the intended memory pool. Additionally, functions that call `default_memory_pool()` should be changed to take a `MemoryPool * `parameter instead.
```
void myFunction(int arg1, int arg2, MemoryPool *pool = default_memory_pool()) {
// function implementation using pool instead of default_memory_pool()
}
// Updated version:
void myFunction(int arg1, int arg2, MemoryPool *pool) {
// function implementation using pool instead of default_memory_pool()
}
// Call site before update:
myFunction(arg1, arg2);
// Updated call site:
MemoryPool *pool = getMemoryPool(); // get the intended memory pool
myFunction(arg1, arg2, pool);
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Reduce number of internal APIs that define default_memory_pool() as default argument value [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on issue #36360:
URL: https://github.com/apache/arrow/issues/36360#issuecomment-1810327874
> This is done by changing the internal functions to not define default_memory_pool() as the default argument and changing the callsites to explicitly pass the intended memory pool.
Yes, of course. The bad consequence of this is that it breaks APIs that are already public. The multi-step approach I suggested in the description (temporary removal of the default + callsite fixes) would at least ensure the Arrow codebase itself is not using `default_memory_pool()` by accident.
Eventually, we could break some of the APIs and drop the `pool = default_memory_pool()` from parameter declarations.
I'm advocating we always pass `default_memory_pool()` explicitly in Arrow code even when a default value is defined in the declaration. That would make us better judge how annoying being explicit becomes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org