@Hans Goudey (HooglyBoogly) Can you nominate one other reviewer from Images and Movies who typically reviews conversions like this as well?

Adding some clang-tidy notes from Hans: P2770

From that paste, it looks like RenderResult's volatile fields are having issues still. The original compiler error the memcpy was trying to fix: godbolt

I see a lot of different ways of allocation that might have different behavior. Eg malloc only allocates, new allocates and constructs. Similar for calloc and cnew. I am not sure if everything was done with the same mindset, what could lead to a higher risk. To reduce risk I would first separate this file in multiple c files. and Only perform the cpp conversion for the parts that you are working on where the idea is to make those parts really cpp-compliant.

I think that the current approach would lead to a lot of c99 code in cpp compile units without a plan to convert them.

Yeah, we need to leave all of the existing mallocs in place. For calloc, we can replace with MEM_cnew as it's the same in that case (and guarded with an "is_trivial" check to ensure the structure can be allocated like that). I think those ones are safe.

And yeah, I did start with separating out the 1 (one!) function I needed to be c++. Then some folks told me to just do the entire file :)

I'll make those other changes tonight. I wasn't sure how much to change as part of the conversion to keep it small vs. leaving it until later.

I think that the current approach would lead to a lot of c99 code in cpp compile units without a plan to convert them.

This is the approach that most C++ conversions in lets say the last 6 months have been taking though , our code style for these conversions is essentially "C with classes" rather than a C++ conversion.

Maybe we should revisit that style, maybe not, but for now to reduce friction and stalls of other work, I'd say let this one just continue on this path, and have that policy making discussion regarding conversions elsewhere.

I think that the current approach would lead to a lot of c99 code in cpp compile units without a plan to convert them.

I prefer this approach since it allows gradual refactoring as time allows. It often doesn't make sense to spend a bunch of time refactoring a file to use C++ types immediately, but at least it's possible after commits like this.
Compiling C code as C++ in the meantime doesn't have any downsides anyway, AFAIK.

Beyond the warnings Jesse linked above, I've looked through this patch and it looks good to me.

Updates for feedback and errors/warnings found on gcc.

offsetof is, apparently, not permitted to use non-constant variables in its expression and this showed up on gcc (but not for clang or msvc). Calculate the offset manually.

The RenderResult volatile fields need handled separately as changing those will ripple to a few other files. For now I removed the old memset and replaced the memcpy with a field-by-field copy instead. There's risk that this will become out of sync so I'll leave a note in the header about this before checkin. Not including that yet in case we don't move forward with this patch.

Code seems good, but still want to test this in more depth before accepting, although the changes are tiny it, code is handled differently and Image touches a lot of areas in Blender. This part isn't well covered with test cases. And just a few devs are actually working here.

@Ray Molenkamp (LazyDodo) @Hans Goudey (HooglyBoogly) if everyone is doing it, it doesn't mean that it is a good idea... Doing it this way introduces additional risks in areas that will not be touched at all by near term projects. Who will be testing StampData, Cryptomatte workflow, Texture painting, RenderData in detail in the near term. By extracting the ImageTile part into a separate compile unit you port stuff over when you actually are developing and testing and it would reduce bugs and lower the stress on devs.

Personally I am in favor of separating into compile units first to reduce risks... what is more in line with the initial approach that @Jesse Yurkovich (deadpin) mentioned.

Yup, good question, see my reply below.

Ultimately I'm here to help and the last thing I want to do is cause trouble and stress. Sometimes just having these conversations is reason enough to help sort out kinks in the process. If that means this review becomes an example of what not to do, then well, at least I helped future souls :)

if everyone is doing it, it doesn't mean that it is a good idea...

Hmm, that wasn't the reasoning I gave... Your other comments are fair though, I suppose.

Personally I am in favor of separating into compile units first to reduce risks... what is more in line with the initial approach that @Jesse Yurkovich (deadpin) mentioned.

If that's preferred, that's fine with me. I just saw this as something that we would want to do eventually, and helpful for other projects, so I offered my time to help review it.
When some code is still using C, it forces the use of wrappers, or makes us compromise the design of other areas just for compatibility.
But totally, as a developer actually working in this area, your opinion matters more in this case.

Fair enough, i'm with hans on this, sorry for wasting your time @Jesse Yurkovich (deadpin)

@Jesse Yurkovich (deadpin) thanks for your constructive positivity.

Think we should discuss this so it becomes part of some common practices with the core module. Don’t get me wrong @Hans Goudey (HooglyBoogly) @Ray Molenkamp (LazyDodo) I have respect for you both and appreciate all the help you do!

Note that before committing please run the tests. On my system it fails inside this patch, but might not be related to this patch and already solved in master.

Oh, we still want to try this? I'll get a bot build going tomorrow as the tests are ok locally.

All platforms pass: https://builder.blender.org/admin/#/builders/18/builds/329