Fix compile error in binaryop/compiled/util.cpp
#10209
Conversation
ttnghia
added
bug
| @@ -45,7 +45,7 @@ struct common_type_functor { | |||
| // Eg. d=t-t | |||
| return data_type{type_to_id<TypeCommon>()}; | |||
| } | |||
| return {}; | |||
| return std::nullopt; | |||
There was a problem hiding this comment.
A default-constructed std::optional should be equivalent to std::nullopt from my reading of cppreference. This pattern is used several times in libcudf.
There was a problem hiding this comment.
There was a problem hiding this comment.
Here is my reasoning:
- The old code
return {}will return a valid optional object with default-constructeddata_type, not anullopt. - Implicitly defined (by the compiler) default constructor of a class does not initialize members of built-in types. (https://stackoverflow.com/questions/2417065/does-the-default-constructor-initialize-built-in-types)
- The class
data_typehas a default constructor, and calling{}will construct a defaultdata_typeobject instead of an empty optional object. - "a default
data_typeobject" does not initialize its members. That's why we got the compile error.
There was a problem hiding this comment.
The old code
return {}will return a valid optional object with default-constructeddata_type, not anullopt.
I don't think so, mate. (1) in the spec indicates otherwise. Here's a trivial godbolt example.
This sounds very much like a compiler bug that we're working around. That said, this change might still be worth having.
There was a problem hiding this comment.
Yeah, the only thing I can think of is a compiler bug that causes return {} to return a valid optional object with default-constructed data_type.
There was a problem hiding this comment.
Hah, so it's compiler bug not just with gcc-10, but gcc <= 10?
We don't see this bug with GCC 9 or 11. There may be some confluence of issues here that cause it to only appear on GCC 10.
There was a problem hiding this comment.
It's warning treated as error. From the links above, I can see that such warning is issued with gcc-7 and 8 too.
There was a problem hiding this comment.
Guys, we have a very hot discussion. Thanks all for your attention 👍 😃
There was a problem hiding this comment.
@ttnghia Can you try moving this function out of the anonymous namespace and see if it compiles on GCC 10 with
return {}?
Moving the function out of anonymous namespace does not help.
There was a problem hiding this comment.
Nothing to get us excited like a possible compiler bug! Appreciate all the voices here - I learned quite a bit. :)
There was a problem hiding this comment.
I'd like to know if return {} compiles without the warning on GCC 10 if the function is not in an anonymous namespace. If that fixes it, I would reference https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86465 in the comment. Otherwise LGTM.
This reverts commit aea7b14.
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #10209 +/- ##
================================================
+ Coverage 10.42% 10.47% +0.04%
================================================
Files 119 122 +3
Lines 20603 20506 -97
================================================
Hits 2148 2148
+ Misses 18455 18358 -97
Continue to review full report at Codecov.
|
|
@gpucibot merge |
This PR fixes a compile error in
binaryop/compiled/util.cpp, when a function with return typestd::optionalreturn{}instead ofstd::nullopt. As such, nvcc 11.5/g++-10.3 yell: