Cleanup: Node internal link relink #104605

Iliya Katushenock · 2023-02-11T04:35:33+01:00

Iliya Katushenock commented

2023-02-11 04:35:33 +01:00

Cleanup: Reduce nested in code. Simplified logic.
Comment formatting.

Cleanup: Reduce nested in code. Simplified logic. Comment formatting.

Iliya Katushenock added 2 commits 2023-02-11 04:35:35 +01:00

07ad2d3406 cleanup node internal reling function

06a9164345 make format

Iliya Katushenock requested review from Hans Goudey 2023-02-11 04:35:43 +01:00

Brecht Van Lommel added this to the Nodes & Physics project 2023-02-13 09:17:56 +01:00

Iliya Katushenock added 2 commits 2023-02-13 20:26:25 +01:00

9a6bfb5a4e Merge branch 'main' into internal_relink

31979794fd fix read after freed

Iliya Katushenock changed title from ~~Cleanup: Node internal link relink function simplified~~ to Fix #104604: Node internal link relink: Avoid delete after freed and some cleanup

2023-02-13 20:38:15 +01:00

Iliya Katushenock added 1 commit 2023-02-15 18:47:10 +01:00

172ba4e7fd Merge branch 'main' into internal_relink

Iliya Katushenock changed title from ~~Fix #104604: Node internal link relink: Avoid delete after freed and some cleanup~~ to Cleanup: Node internal link relink

2023-02-15 19:06:13 +01:00

Iliya Katushenock added 1 commit 2023-02-15 19:06:54 +01:00

58ca215b1e Back to just cleanup

Hans Goudey requested changes 2023-02-16 20:40:12 +01:00

Hans Goudey left a comment

I'm sorry to be picky about this, since I know it's just a fairly small cleanup, but I don't think this is really how the process should work. Combining all of these different cleanups in a single commit makes the diff hard to read. For this sort of change, the diff should be obvious and clearly correct.

The commit includes:

Use const arguments
Use references instead of pointers
Comment formatting
Variable renames
Reducing indentation
Whitespace changes

I agree with all of the changes you've made, but in the end, they're applied arbitrarily to this single function, and applied all at once, which I mentioned earlier. Instead, I'd rather see commits that apply these cleanups more broadly-- for example, "Cleanup: Comment formatting in node.cc" or "Cleanup: Use references in node.cc" or "Cleanup: Standardize BKE_node.h function naming", etc-- changes that are ore obvious and less local.

I'm sorry to be picky about this, since I know it's just a fairly small cleanup, but I don't think this is really how the process should work. Combining all of these different cleanups in a single commit makes the diff hard to read. For this sort of change, the diff should be *obvious* and clearly correct. The commit includes: - Use const arguments - Use references instead of pointers - Comment formatting - Variable renames - Reducing indentation - Whitespace changes I agree with all of the changes you've made, but in the end, they're applied arbitrarily to this single function, and applied all at once, which I mentioned earlier. Instead, I'd rather see commits that apply these cleanups more broadly-- for example, "Cleanup: Comment formatting in node.cc" or "Cleanup: Use references in node.cc" or "Cleanup: Standardize BKE_node.h function naming", etc-- changes that are ore obvious and less local.

👀 1

Iliya Katushenock closed this pull request

2023-02-19 00:38:25 +01:00

Iliya Katushenock deleted branch internal_relink

2023-02-19 00:38:31 +01:00

Hans Goudey referenced this pull request

2023-02-19 23:17:48 +01:00

Cleanup: Use const arguments in node.cc #104937