NFC: Remove `Vector::From` and replace it with aggregate initializati… by ruevs · Pull Request #1703 · solvespace/solvespace

ruevs · 2026-03-30T13:30:41Z

…on {}

...in all places where it is possible.

The 34 remaining places either initialize a vector from handles or call a method of the resulting vector object. For example Vector::From(x, y, z).WithMagnitude(1.0)

Explanation

Now that 3.2. is released and we have some time to potentially break things here is my "intrusive change".

[sorry for the massive spam]
@jwesthues @phkahler @rpavlik @vespakoen @ppd, this is something that has been bothering me for a while. It is the typical change that @whitequark would not have accepted - because it touches many, many files and "muddies" the history. So please consider this just a proposal and let's discuss it.

It is an "almost NonFunctionalChange" except that constants are replaced with VERY_POSITIVE and VERY_NEGATIVE in a few places - an the values may not be exactly the same.

@iscgar too, take a look at this - you seem to be the right type of person to give an opinion on it :-)

phkahler · 2026-03-30T14:28:54Z

Overall I think this improves readability. This is a C++11 feature.

rpavlik

Seems reasonable enough to me. If it's too noisy in the blame, can always use git blame ignore refs file thingie. As long as it actually worked (I know there are some slight differences between using this and calling a constructor with 3 params, e.g. fewer (or no?) implicit conversions happen, etc.), it looks nice to me. Less visual noise

…on {} ...in all places where it is possible. The 34 remaining places either initialize a vector from handles or call a method of the resulting vector object. For example `Vector::From(x, y, z).WithMagnitude(1.0)`

vespakoen · 2026-03-31T07:54:41Z

I think this change is fine.
The one thing I am wondering is: why have those comments with // {0, 0, 0} ? Might as well initialize it like that directly?

For example:

Vector v = {}; // {0, 0, 0}

Why not:

Vector v = {0, 0, 0};

?

…ation ... and making the `ARC_LINE_LEN_RATIO ARC_LINE_DIFFERENCE` case similar to `EQ_LEN_PT_LINE_D` below it.

ruevs · 2026-03-31T08:32:24Z

I think this change is fine. The one thing I am wondering is: why have those comments with // {0, 0, 0} ? Might as well initialize it like that directly?

For example:

Vector v = {}; // {0, 0, 0}

Why not:

Vector v = {0, 0, 0};

?

To minimize the difference and make review easier ;-) Otherwise the "right thing" to do is just {}.

ruevs · 2026-03-31T08:42:38Z

As long as it actually worked (I know there are some slight differences between using this and calling a constructor with 3 params, e.g. fewer (or no?) implicit conversions happen, etc.),

It works - in the sense - opens, loads a few simple models, saves... Hopefully it does not hide some monster :-)

it looks nice to me. Less visual noise

This is the reason I did it as well.

ruevs · 2026-03-31T12:25:46Z

I think this change is fine. The one thing I am wondering is: why have those comments with // {0, 0, 0} ? Might as well initialize it like that directly?
For example:
Vector v = {}; // {0, 0, 0}
Why not:
Vector v = {0, 0, 0};
?

To minimize the difference and make review easier ;-) Otherwise the "right thing" to do is just {}.

@vespakoen After thinking more about your question I think that, while having the /*{0, 0, 0}*/ comments was helpful for review - to make sure that {} is used only in the places where we need a zero vector - ultimately it is stupid. Everyone knows about {} zero initialization.

So I added two more commits:

Converts all the remaining {0, 0, 0} to {} and adds the (stupid) comment - just for review.
Removes all of the /*{0, 0, 0}*/ comments.

And of course all of this should be squashed into one commit if we decide to merge it.

Edit:

This is easy to review even as a single change (in the "Files changed" tab). I went through everything one more time and noticed one more inconsistency (which I introduced in these change series):

When declaring a Vector variable and initializing it, we can do it in two ways

Vector abcd = {1, 2, 3}; Vector nothing = {};
Vector abcd{1, 2, 3}; Vector nothing{};

I do realize that the two are subtly different in semantics in the language. Which one do you guys/gals prefer?

…ion.

phkahler · 2026-03-31T19:07:53Z

When declaring a Vector variable and initializing it, we can do it in two ways

Vector abcd = {1, 2, 3}; Vector nothing = {};

Vector abcd{1, 2, 3}; Vector nothing{};

I do realize that the two are subtly different in semantics in the language. Which one do you guys/gals prefer?

I'm a fan of explicit, so of those two I prefer the = {}; I'm also kind of a fan of = {0, 0, 0} although that bothers me because it's an implicit conversion to double ;-) And using 0.0 would be way too verbose.

ruevs · 2026-03-31T19:46:02Z

I'm also kind of a fan of = {0, 0, 0} although that bothers me because it's an implicit conversion to double ;-)

Indeed the first time I started doing this I started changing all constants to 0., 1. etc... because the implicit conversion bothers me as well... then I have up. So I like {} - at least it gets rid of some of the integers :-)

iscgar · 2026-03-31T19:53:58Z

I do like to use aggregate initialisation where possible. But I think it depends on the specific use case, and in some cases that I can see (such as in the case of tr.an.EqualsExactly({})) it makes the intention less clear IMO. Overall I think this is a good change, although I don't think it makes a huge difference one way or the other, so whatever is easier for you guys is what I think you should go with.

EDIT: aside from the places where an empty initialisation list is passed to a function, which makes it less clear that the argument is a zero vector, another thing that I think could be improved with this change is the inconsistency wrt variable initialisation with the zero vector. In some places an assignment is used, whereas in others a direct initialisation syntax is used, and I think it would be best to use a single style throughout the code.

As for muddying the history... while this argument has some merit, I don't think it should be used to block changes that modernise the code, as it would just lead to either stagnation or inconsistency when new code is introduced, which I think is even worse (also, judging from the the little history that I've seen, whitequark did accept rpavlik's ranged-for iteration changes, which also touched basically every source file, and for the most part were non functional, so I think at some point such a change might have been accepted as well 😃)

ruevs · 2026-04-04T13:50:48Z

in the case of tr.an.EqualsExactly({})) it makes the intention less clear IMO.

I considered that as well. For the same reason I left the {0, 0, 0} in places where matrixes are initialized.

I shall change the function({}) locations.

…of {} In addition unify `Vector` variable initialization style.

ruevs · 2026-04-04T18:48:19Z

Added on more commit:

When passing a zero vector as a parameter use {0, 0, 0} instead of {}
In addition unify Vector variable initialization style.

So shall we {squash and) merge this?

phkahler · 2026-04-07T17:45:34Z

So shall we {squash and) merge this?

Yes

ruevs requested review from jwesthues, phkahler, ppd, rpavlik and vespakoen March 30, 2026 13:30

phkahler reviewed Mar 30, 2026

View reviewed changes

Comment thread src/srf/shell.cpp Outdated

Comment thread src/constrainteq.cpp

Comment thread src/drawconstraint.cpp Outdated

rpavlik reviewed Mar 30, 2026

View reviewed changes

NFC: Remove Vector::From and replace it with aggregate initializati…

9a9208d

…on {} ...in all places where it is possible. The 34 remaining places either initialize a vector from handles or call a method of the resulting vector object. For example `Vector::From(x, y, z).WithMagnitude(1.0)`

NFC: Remove one more Vector::From by removing unnecessary initializ…

ad540f0

…ation ... and making the `ARC_LINE_LEN_RATIO ARC_LINE_DIFFERENCE` case similar to `EQ_LEN_PT_LINE_D` below it.

ruevs force-pushed the JihadOnVectorFrom branch from 81782ee to ad540f0 Compare March 31, 2026 08:00

ruevs added 2 commits March 31, 2026 15:27

NFC: Raplace all aggregate initializations {0, 0, 0} with {}

5a471b5

NFC: Ramove all {0, 0, 0} comments. Everyone knows zero initializat…

c4ceeb5

…ion.

ruevs force-pushed the JihadOnVectorFrom branch from 97d9588 to c4ceeb5 Compare March 31, 2026 12:27

NFC: When passing a zero vector as a parameter use {0, 0, 0} instead …

1464c4b

…of {} In addition unify `Vector` variable initialization style.

ruevs merged commit 289651e into solvespace:master Apr 8, 2026
4 checks passed

ruevs deleted the JihadOnVectorFrom branch April 8, 2026 02:37

ruevs added the internals label Apr 8, 2026

Conversation

ruevs commented Mar 30, 2026

Explanation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phkahler commented Mar 30, 2026

Uh oh!

rpavlik left a comment

Choose a reason for hiding this comment

Uh oh!

vespakoen commented Mar 31, 2026

Uh oh!

ruevs commented Mar 31, 2026

Uh oh!

ruevs commented Mar 31, 2026

Uh oh!

ruevs commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Edit:

Uh oh!

phkahler commented Mar 31, 2026

Uh oh!

ruevs commented Mar 31, 2026

Uh oh!

iscgar commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruevs commented Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruevs commented Apr 4, 2026

Uh oh!

phkahler commented Apr 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ruevs commented Mar 31, 2026 •

edited

Loading

iscgar commented Mar 31, 2026 •

edited

Loading

ruevs commented Apr 4, 2026 •

edited

Loading