You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "raulcd (via GitHub)" <gi...@apache.org> on 2024/11/19 12:41:19 UTC
[PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
raulcd opened a new pull request, #44789:
URL: https://github.com/apache/arrow/pull/44789
### Rationale for this change
I was getting some `-Wmaybe-uninitialized` warnings locally when building, see original issue for log.
### What changes are included in this PR?
A couple of minor changes to avoid the warnings, initializing a variable and removing an unnecessary one.
### Are these changes tested?
Code will be tested via CI and I can confirm I can compile without warnings now:
```
$ cmake --build arrow/cpp/build --target install -- -j 8
[411/412] Install the project...
-- Install configuration: "RELEASE"
```
### Are there any user-facing changes?
No
--
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: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #44789:
URL: https://github.com/apache/arrow/pull/44789#discussion_r1852237967
##########
cpp/src/arrow/util/iterator.h:
##########
@@ -292,18 +292,17 @@ class TransformIterator {
finished_ = true;
return next_res.status();
}
- auto next = *next_res;
Review Comment:
I tested and it also gets rid of the warning. Thanks both!
--
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: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #44789:
URL: https://github.com/apache/arrow/pull/44789#issuecomment-2491376267
> @raulcd Unrelated to this PR, but you probably want to compile in Debug mode when developing Arrow C++.
Thanks for sharing!
I really thought I was using the `ninja-debug-python` preset but it seems I forgot to update when copy&pasting the documentation from which defaults to `ninja-release-python`: https://github.com/apache/arrow/blob/27bbd593625122a4a25d9471c8aaf5df54a6dcf9/docs/source/developers/python.rst?plain=1#L305-L307
Maybe we should default the commands to build on debug on the development docs and mention that it can be changed to release instead of the current?
--
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: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44789:
URL: https://github.com/apache/arrow/pull/44789#discussion_r1850706599
##########
cpp/src/arrow/util/iterator.h:
##########
@@ -292,18 +292,17 @@ class TransformIterator {
finished_ = true;
return next_res.status();
}
- auto next = *next_res;
Review Comment:
Given this line:
```
inlined from ‘arrow::Result<T>::Result(U&&) [with U = std::shared_ptr<arrow::Buffer>; E = void; T = std::optional<std::shared_ptr<arrow::Buffer> >]’ at /home/raulcd/code/arrow/cpp/src/arrow/result.h:165:19,
```
it seems that we're implicitly converting from `U` to `Result<std::optional<U>>` and that's too much for the compiler to swallow?
We don't necessarily need to care about this warning. That said, I agree that the following would be better anyway:
```c++
auto next = std::move(*next_res);
```
--
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: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44789:
URL: https://github.com/apache/arrow/pull/44789#issuecomment-2485610250
:warning: GitHub issue #44788 **has been automatically assigned in GitHub** to PR creator.
--
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: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #44789:
URL: https://github.com/apache/arrow/pull/44789
--
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: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #44789:
URL: https://github.com/apache/arrow/pull/44789#issuecomment-2493018125
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2e1ddf5069731d2b908b18d851b2389973cc988c.
There were 132 benchmark results with an error:
- Commit Run on `arm64-t4g-2xlarge-linux` at [2024-11-22 02:34:14Z](https://conbench.ursa.dev/compare/runs/f7a4e275e70f4ea3ac44ae5ae22d58f0...74f848f97d784946bb0fe6fd0eb3a0af/)
- [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-07, scale_factor=1](https://conbench.ursa.dev/benchmark-results/0673ffaf222d770e80003777230aa384)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-09, scale_factor=1](https://conbench.ursa.dev/benchmark-results/0673ffb183ac72438000e095ba02082f)
- and 130 more (see the report linked below)
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/33366295324) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.
--
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: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #44789:
URL: https://github.com/apache/arrow/pull/44789#issuecomment-2489148872
@raulcd Unrelated to this PR, but you probably want to compile in Debug mode when developing Arrow 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #44789:
URL: https://github.com/apache/arrow/pull/44789#issuecomment-2491399138
> Maybe we should default the commands to build on debug on the development docs and mention that it can be changed to release instead of the current?
Ah, probably indeed.
--
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: [PR] GH-44788: [C++] Fix a couple of maybe-uninitialized warnings [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #44789:
URL: https://github.com/apache/arrow/pull/44789#discussion_r1849349802
##########
cpp/src/arrow/util/iterator.h:
##########
@@ -292,18 +292,17 @@ class TransformIterator {
finished_ = true;
return next_res.status();
}
- auto next = *next_res;
Review Comment:
Hmm. I feel that this is strange...
@pitrou Do you have any idea how to suppress this warning?
```text
[47/311] Building CXX object src/arrow/CMakeFiles/arrow_csv.dir/csv/reader.cc.o
In file included from /usr/include/c++/13/bits/shared_ptr.h:53,
from /usr/include/c++/13/memory:80,
from /home/raulcd/code/arrow/cpp/src/arrow/csv/reader.h:20,
from /home/raulcd/code/arrow/cpp/src/arrow/csv/reader.cc:18:
In constructor ‘std::__shared_ptr<_Tp, _Lp>::__shared_ptr(std::__shared_ptr<_Tp, _Lp>&&) [with _Tp = arrow::Buffer; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]’,
inlined from ‘std::shared_ptr<_Tp>::shared_ptr(std::shared_ptr<_Tp>&&) [with _Tp = arrow::Buffer]’ at /usr/include/c++/13/bits/shared_ptr.h:360:41,
inlined from ‘constexpr std::_Optional_payload_base<_Tp>::_Storage<_Up, false>::_Storage(std::in_place_t, _Args&& ...) [with _Args = {std::shared_ptr<arrow::Buffer>}; _Up = std::shared_ptr<arrow::Buffer>; _Tp = std::shared_ptr<arrow::Buffer>]’ at /usr/include/c++/13/optional:244:8,
inlined from ‘constexpr std::_Optional_payload_base<_Tp>::_Optional_payload_base(std::in_place_t, _Args&& ...) [with _Args = {std::shared_ptr<arrow::Buffer>}; _Tp = std::shared_ptr<arrow::Buffer>]’ at /usr/include/c++/13/optional:126:4,
inlined from ‘constexpr std::_Optional_payload<std::shared_ptr<arrow::Buffer>, true, false, false>::_Optional_payload(std::in_place_t, _Args&& ...) [with _Args = {std::shared_ptr<arrow::Buffer>}][inherited from std::_Optional_payload_base<std::shared_ptr<arrow::Buffer> >]’ at /usr/include/c++/13/optional:399:42,
inlined from ‘constexpr std::_Optional_payload<std::shared_ptr<arrow::Buffer>, false, false, false>::_Optional_payload(std::in_place_t, _Args&& ...) [with _Args = {std::shared_ptr<arrow::Buffer>}][inherited from std::_Optional_payload_base<std::shared_ptr<arrow::Buffer> >]’ at /usr/include/c++/13/optional:433:57,
inlined from ‘constexpr std::_Optional_base<_Tp, <anonymous>, <anonymous> >::_Optional_base(std::in_place_t, _Args&& ...) [with _Args = {std::shared_ptr<arrow::Buffer>}; typename std::enable_if<is_constructible_v<_Tp, _Args ...>, bool>::type <anonymous> = false; _Tp = std::shared_ptr<arrow::Buffer>; bool <anonymous> = false; bool <anonymous> = false]’ at /usr/include/c++/13/optional:523:4,
inlined from ‘constexpr std::optional<_Tp>::optional(_Up&&) [with _Up = std::shared_ptr<arrow::Buffer>; typename std::enable_if<__and_v<std::__not_<std::is_same<std::optional<_Tp>, typename std::remove_cv<typename std::remove_reference<_Tuple>::type>::type> >, std::__not_<std::is_same<std::in_place_t, typename std::remove_cv<typename std::remove_reference<_Tuple>::type>::type> >, std::is_constructible<_T1, _U1>, std::is_convertible<_U1, _T1> >, bool>::type <anonymous> = true; _Tp = std::shared_ptr<arrow::Buffer>]’ at /usr/include/c++/13/optional:751:47,
inlined from ‘void arrow::internal::AlignedStorage<T>::construct(A&& ...) [with A = {std::shared_ptr<arrow::Buffer>}; T = std::optional<std::shared_ptr<arrow::Buffer> >]’ at /home/raulcd/code/arrow/cpp/src/arrow/util/aligned_storage.h:52:5,
inlined from ‘void arrow::Result<T>::ConstructValue(U&&) [with U = std::shared_ptr<arrow::Buffer>; T = std::optional<std::shared_ptr<arrow::Buffer> >]’ at /home/raulcd/code/arrow/cpp/src/arrow/result.h:448:23,
inlined from ‘arrow::Result<T>::Result(U&&) [with U = std::shared_ptr<arrow::Buffer>; E = void; T = std::optional<std::shared_ptr<arrow::Buffer> >]’ at /home/raulcd/code/arrow/cpp/src/arrow/result.h:165:19,
inlined from ‘arrow::Result<std::optional<_Up> > arrow::TransformIterator<T, V>::Pump() [with T = std::shared_ptr<arrow::Buffer>; V = std::shared_ptr<arrow::Buffer>]’ at /home/raulcd/code/arrow/cpp/src/arrow/util/iterator.h:306:27:
/usr/include/c++/13/bits/shared_ptr_base.h:1532:9: warning: ‘*(std::__shared_ptr<arrow::Buffer, __gnu_cxx::_S_atomic>*)((char*)&next + offsetof(arrow::TransformFlow<std::shared_ptr<arrow::Buffer> >,arrow::TransformFlow<std::shared_ptr<arrow::Buffer> >::yield_value_.std::optional<std::shared_ptr<arrow::Buffer> >::<unnamed>.std::_Optional_base<std::shared_ptr<arrow::Buffer>, false, false>::<unnamed>)).std::__shared_ptr<arrow::Buffer, __gnu_cxx::_S_atomic>::_M_ptr’ may be used uninitialized [-Wmaybe-uninitialized]
1532 | : _M_ptr(__r._M_ptr), _M_refcount()
| ^~~~~~~~~~~~~~~~~~
In file included from /home/raulcd/code/arrow/cpp/src/arrow/record_batch.h:30,
from /home/raulcd/code/arrow/cpp/src/arrow/csv/reader.h:24:
/home/raulcd/code/arrow/cpp/src/arrow/util/iterator.h: In member function ‘arrow::Result<std::optional<_Up> > arrow::TransformIterator<T, V>::Pump() [with T = std::shared_ptr<arrow::Buffer>; V = std::shared_ptr<arrow::Buffer>]’:
/home/raulcd/code/arrow/cpp/src/arrow/util/iterator.h:295:12: note: ‘*(std::__shared_ptr<arrow::Buffer, __gnu_cxx::_S_atomic>*)((char*)&next + offsetof(arrow::TransformFlow<std::shared_ptr<arrow::Buffer> >,arrow::TransformFlow<std::shared_ptr<arrow::Buffer> >::yield_value_.std::optional<std::shared_ptr<arrow::Buffer> >::<unnamed>.std::_Optional_base<std::shared_ptr<arrow::Buffer>, false, false>::<unnamed>)).std::__shared_ptr<arrow::Buffer, __gnu_cxx::_S_atomic>::_M_ptr’ was declared here
295 | auto next = *next_res;
| ^~~~
```
See also: https://github.com/apache/arrow/issues/44788
`auto next = std::move(*next_res)`?
--
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