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.

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(ustring): fix Cuda warnings #3978

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 8, 2023

To my eyes, these two constructors look like they should be treated as doing exactly the same thing:

MyType()
    : mymember(...)
{ }

and

MyType()
{
   mymember = ...;
}

But nvcc in Cuda mode says no, if the constructor is constexpr, the former is okey dokey, but latter gives a warning. Fine, Cuda, whatver you say.

To my eyes, these two constructors look like they should be treated as
doing exactly the same thing:

    MyType()
        : mymember(...)
    { }

and

    MyType()
    {
       mymember = ...;
    }

But nvcc in Cuda mode says no, if the constructor is constexpr,
the former is okey dokey, but latter gives a warning. Fine, Cuda,
whatver you say.

Signed-off-by: Larry Gritz <[email protected]>
Copy link
Collaborator

@ThiagoIze ThiagoIze left a comment

Choose a reason for hiding this comment

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

I think this was an issue in C++ as well until c++20, as we can see in these c++ examples: https://gcc.godbolt.org/z/63n7n36os with clang and gcc.

I don't know if this is a good idea, but you could create a macro (and then undef it later) that does the Strutil::strhash(str) / ustring(str).hash() and then you won't need all these ifdefs and comments in the various ustringhash constructors. This might not be worth it for just three places, so I'm approving it as is.

@lgritz
Copy link
Collaborator Author

lgritz commented Sep 8, 2023

Yes. The warning specifically says "this is not allowed until C++20". Still surprising to me that even before C++20, any compiler or the language would treat unconditional assignment of the sole member variable inside the braces as being any different than the : foo(value) member initialization. I mean, sure, it's a different syntactic construct, but in my brain it has to result in the same code, so it should be the same rules about whether it's allowed, right?

@lgritz lgritz merged commit d795a3b into AcademySoftwareFoundation:master Sep 8, 2023
23 checks passed
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Sep 8, 2023
To my eyes, these two constructors look like they should be treated as
doing exactly the same thing:

    MyType()
        : mymember(...)
    { }

and

    MyType()
    {
       mymember = ...;
    }

But nvcc in Cuda mode says no, if the constructor is constexpr, the
former is okey dokey, but latter gives a warning. Fine, Cuda, whatver
you say.

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Sep 8, 2023
To my eyes, these two constructors look like they should be treated as
doing exactly the same thing:

    MyType()
        : mymember(...)
    { }

and

    MyType()
    {
       mymember = ...;
    }

But nvcc in Cuda mode says no, if the constructor is constexpr, the
former is okey dokey, but latter gives a warning. Fine, Cuda, whatver
you say.

Signed-off-by: Larry Gritz <[email protected]>
@lgritz lgritz deleted the lg-ustring branch September 8, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants