Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compile error in binaryop/compiled/util.cpp #10209

Merged
merged 5 commits into from
Feb 3, 2022
Merged

Fix compile error in binaryop/compiled/util.cpp #10209

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
07df129
Fix compile error
ttnghia Feb 3, 2022
758cc53
Add comment
ttnghia Feb 3, 2022
f21ebac
Fix comment typo
ttnghia Feb 3, 2022
aea7b14
Add specific GCC version to clarify the bug
ttnghia Feb 3, 2022
fbae4ea
Revert "Add specific GCC version to clarify the bug"
ttnghia Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cpp/src/binaryop/compiled/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ struct common_type_functor {
// Eg. d=t-t
return data_type{type_to_id<TypeCommon>()};
}
return {};

// A compiler bug may cause a compilation error when using empty initializer list to construct
ttnghia marked this conversation as resolved.
Show resolved Hide resolved
// an std::optional object containing no `data_type` value. Therefore, we should explicitly
// return `std::nullopt` instead.
return std::nullopt;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A default-constructed std::optional should be equivalent to std::nullopt from my reading of cppreference. This pattern is used several times in libcudf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this: https://stackoverflow.com/questions/57964217/stdoptional-construct-empty-with-or-stdnullopt

Copy link
Contributor Author

@ttnghia ttnghia Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my reasoning:

  • The old code return {} will return a valid optional object with default-constructed data_type, not a nullopt.
  • 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_type has a default constructor, and calling {} will construct a default data_type object instead of an empty optional object.
  • "a default data_type object" does not initialize its members. That's why we got the compile error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code return {} will return a valid optional object with default-constructed data_type, not a nullopt.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's warning treated as error. From the links above, I can see that such warning is issued with gcc-7 and 8 too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guys, we have a very hot discussion. Thanks all for your attention 👍 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@bdice bdice Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to get us excited like a possible compiler bug! Appreciate all the voices here - I learned quite a bit. :)

}
};
template <typename TypeLhs, typename TypeRhs>
Expand Down