You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/10 04:03:11 UTC
[GitHub] [arrow] westonpace opened a new pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
westonpace opened a new pull request #9975:
URL: https://github.com/apache/arrow/pull/9975
Gandiva build failure caused by failure of GCC 4.8.2 compiler to automatically move return value. This is rather troubling. I'm not super happy with the fix because it is not needed (and potentially non-optimal because it can prevent RVO) in later compilers and looks weird. I'm open to suggestions.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 closed pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
cyb70289 closed pull request #9975:
URL: https://github.com/apache/arrow/pull/9975
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-817325048
> I'm open to suggestions
Maybe add a comment to this effect so that whenever we remove support for gcc < 4.9, we can remove this? Or could you even use an `#ifdef` or something to only do this on gcc < 4.9?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-823540217
@cyb70289 I updated line 95 and fixed up the commit message. @pitrou is correct that we do not need to worry about RVO if the output type differs so it should be harmless (i.e. no need for an ifdef).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-823690863
Can we merge this to include this into 4.0.0 RC2?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-819601167
I agree, an ifdef could be OK, though right now this is only used in tests.
I submitted https://github.com/lidavidm/crossbow/actions/runs/748817979 for now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#discussion_r614550187
##########
File path: cpp/src/arrow/util/vector.h
##########
@@ -130,7 +130,7 @@ Result<std::vector<T>> UnwrapOrRaise(std::vector<Result<T>>&& results) {
}
out.push_back(it->MoveValueUnsafe());
}
- return out;
+ return std::move(out);
Review comment:
Looks to me this `std::move` does no harm.
Regarding RVO, as we are returning `Result` wrapped object, not the object itself, to my knowledge, there must be a temporary object to be moved or copied by `Result`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#discussion_r615233852
##########
File path: cpp/src/arrow/util/vector.h
##########
@@ -130,7 +130,7 @@ Result<std::vector<T>> UnwrapOrRaise(std::vector<Result<T>>&& results) {
}
out.push_back(it->MoveValueUnsafe());
}
- return out;
+ return std::move(out);
Review comment:
Agreed with @cyb70289 . RVO is only guaranteed if the returned value has the exact return type declared by the function.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-821754387
I'll side with @cyb70289 and @lidavidm that an ifdef would probably be overkill. Feel free to merge.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-817073669
https://issues.apache.org/jira/browse/ARROW-12325
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#discussion_r614749123
##########
File path: cpp/src/arrow/util/vector.h
##########
@@ -130,7 +130,7 @@ Result<std::vector<T>> UnwrapOrRaise(std::vector<Result<T>>&& results) {
}
out.push_back(it->MoveValueUnsafe());
}
- return out;
+ return std::move(out);
Review comment:
I agree, it's harmless and not actively used, so this one instance is not really something to worry over much about. Thinking on it more I think I was more surprised we get away with it everywhere else. We are returning `Result` most of the time and we don't usually move return 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#discussion_r614553629
##########
File path: cpp/src/arrow/util/vector.h
##########
@@ -130,7 +130,7 @@ Result<std::vector<T>> UnwrapOrRaise(std::vector<Result<T>>&& results) {
}
out.push_back(it->MoveValueUnsafe());
}
- return out;
+ return std::move(out);
Review comment:
cc @pitrou @emkornfield
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#discussion_r615327919
##########
File path: cpp/src/arrow/util/vector.h
##########
@@ -130,7 +130,7 @@ Result<std::vector<T>> UnwrapOrRaise(std::vector<Result<T>>&& results) {
}
out.push_back(it->MoveValueUnsafe());
}
- return out;
+ return std::move(out);
Review comment:
@westonpace , also change line 95? And maybe tweak commit message a bit?
I do see `return std::move(local_obj)` is used extensively when returning `Result` in other source files.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm edited a comment on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
lidavidm edited a comment on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-819601167
I agree, an ifdef could be OK, though right now this is only used in tests.
This builds/passes tests: https://github.com/lidavidm/crossbow/actions/runs/748817979 (it fails on uploading but we don't care about that).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-823698502
The build failures appear to all be expected. MacOS is failing due to LLVM (ARROW-12467). C GLib & Ruby is failing on Windows with segmentation fault but was doing so before this (we should probably create a JIRA). The JNI failure is ARROW-11633
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #9975: ARROW-12325: [C++] [CI] Nightly gandiva build failing due to failure of compiler to move return value
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #9975:
URL: https://github.com/apache/arrow/pull/9975#issuecomment-821770304
ifdef looks more clear to show our intention.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org