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

Use delegating constructors (C++11 feature) #1489

Closed
dimitry-ishenko opened this issue Apr 1, 2024 · 4 comments
Closed

Use delegating constructors (C++11 feature) #1489

dimitry-ishenko opened this issue Apr 1, 2024 · 4 comments
Assignees
Milestone

Comments

@dimitry-ishenko
Copy link
Contributor

solid_color already uses it. Following classes could benefit from it:

  • checker_texture
  • constant_medium
  • ray
  • lambertian
  • diffuse_light
  • isotropic

Constructors of the following classes can be merged:

  • sphere -- combine moving and static constructors
  • noise_texture -- default constructor results in uninitialized use of the scale data member; combine both constructors and provide default value for scale
@dimitry-ishenko dimitry-ishenko changed the title Use deleting constructors (C++11 feature) Use delegating constructors (C++11 feature) Apr 1, 2024
@hollasch hollasch self-assigned this Apr 2, 2024
@hollasch
Copy link
Collaborator

hollasch commented Apr 2, 2024

For consideration to pull into v4.0.0.

Peter's already covered the philosophical fork with the moving vs static sphere constructor, and I prefer the choice he made, and think that right now the two constructors (single center or double center) are good as they are.

I agree with everything else, and in addition would like to review a lot of our nonsense default constructors. That is instances that by default are either garbage or zombie objects. For example, our code is fine with deleting the default noise_texture constructor. We do need things like a default ray constructor, but should just delegate to the standard constructor with reasonable default values. This can hurt performance in some cases, but not really for our purposes.

@hollasch hollasch added this to the v4.0.0 milestone Apr 2, 2024
@dimitry-ishenko
Copy link
Contributor Author

and in addition would like to review a lot of our nonsense default constructors.

IMHO, for some of the basic types such as vec3, ray, interval, etc, which act as POD (plain-old data) types, there is no need to have constructors at all. Consider the following example:

struct vec3
{
    double data[3];

    auto&& operator[](std::size_t i) const { return data[i]; }
    auto&& operator[](std::size_t i) { return data[i]; }
};

int main()
{
    vec3 v1;
    vec3 v2{ };

    std::cout << "v1 = " << v1 << '\n';
    std::cout << "v2 = " << v2 << '\n';

    int i1;
    int i2{ };

    std::cout << "i1 = " << i1 << '\n';
    std::cout << "i2 = " << i2 << '\n';

    vec3 v3{1, 2, 3};
    std::cout << "v3 = " << v3 << '\n';
}

Output:

v1 = 0 6.92724e-310 7.87263e-85
v2 = 0 0 0
i1 = -574343832
i2 = 0
v3 = 1 2 3

Now vec3 acts the same way as built-in types such as int, double, etc. In other words, vec3 v1; creates uninitialized instance, while vec3 v2{ }; creates instance with zero-initialized elements (ie, same as vec3{0, 0, 0}). At the same time, you can initialize the elements of vec3 using brace-initialization, as in vec3 v3{1, 2, 3};. This is all part of the uniform initialization idiom that was introduced in C++11.

@dimitry-ishenko
Copy link
Contributor Author

Here is the link that demonstrates the above: https://godbolt.org/z/cq1P51z3f

@hollasch
Copy link
Collaborator

Yes, I would definitely take advantage of the modern C++ idioms in my own C++ implementation. Beyond the scope of these books, though.

hollasch added a commit that referenced this issue Apr 13, 2024
Found two places where delegating constructors helped: ray and
checker_texture.

In addition, this includes some minor constructor cleanup.

Resolves #1489
hollasch added a commit that referenced this issue Apr 16, 2024
Found two places where delegating constructors helped: ray and
checker_texture.

In addition, this includes some minor constructor cleanup.

Resolves #1489
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

No branches or pull requests

2 participants