Skip to content

Remove unnecessary null pointer checks (ASWF issue #257) #2046

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

epowers
Copy link

@epowers epowers commented May 16, 2025

Fix remove unnecessary null pointer checks, issue #257.

There only appeared to be one obvious case left.

Code search process:

grep -C 3 delete .

... then look for if statements, in C-like code, conditioned on the same variable, and no other obvious purpose that required the if statement.

See:

#257

…dation#257.

There only appeared to be one obvious case left.

Code search process:

    grep -C 3 delete .

... then look for if statements, in C-like code, conditioned on the same
variable, and no other obvious purpose that required the if statement.

Signed-off-by: Eric Powers <[email protected]>
@epowers epowers requested a review from kmuseth as a code owner May 16, 2025 21:41
Copy link

linux-foundation-easycla bot commented May 16, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: epowers / name: Eric Powers (135616d)

@epowers
Copy link
Author

epowers commented May 16, 2025

CLA Not Signed...

working this internally...

Copy link
Contributor

@jmlait jmlait left a comment

Choose a reason for hiding this comment

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

Thank you for the attention to detail!

The change to Grid Builder seems good; but I'd not go with the change to the reference code.

@@ -1968,7 +1968,7 @@ void Grid<BuildT>::operator()(const Func& func, const CoordBBox& bbox, ValueType
NANOVDB_ASSERT(leaf == nullptr);
}
}// loop over sub-part of leafBBox
if (leaf) delete leaf;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, the ghost of past compilers continue to haunt us.

Thankfully modern compilers and language specs have unified so we can reliably not test for null before deleting.

@@ -2985,7 +2985,6 @@ vdbJSON()
GA_PrimitiveJSON* json = new geo_PrimVDBJSON;
if (nullptr != theJSON.compare_swap(nullptr, json)) {
delete json;
json = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

It is correct that this is unnecessary as written.

However, future code COULD be written after the if(); which would then have a stale pointer. So there is a potential benefit.

I do not think there is a harm in execution time; as the compiler should easily reason that this stack variable is unused prior to leaving scope and remove the assignment automatically.

So while I wouldn't advocate necessarily writing this in the first place, I do not think it is something we should remove. (it is even possible some earlier version of the code used json afterwards? So if someone brings back that; we want to keep the assignment?)

In any case, this is under the "reference" section because we do not actively compile this as part of the OpenVDB library, as it is kept as a mirror of the internal Houdini code.

Copy link
Author

Choose a reason for hiding this comment

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

You're right... that's a good pattern to repeat.

Copy link
Author

Choose a reason for hiding this comment

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

Force pushed with only the one commit, for review @jmlait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants