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

Properly testing pointer vars aren't null before using them. #1378

Merged
merged 2 commits into from Aug 23, 2015

Conversation

Projects
None yet
3 participants
@Areloch
Contributor

Areloch commented Jul 29, 2015

Issue found by PVS Studio:

Several instances where we utilize a pointer variable without properly testing that they aren't null first.

Issue found by PVS Studio:
Several instances where we utilize a pointer variable without properly testing that they aren't null first.

@Areloch Areloch added this to the 3.8 milestone Jul 29, 2015

@Areloch Areloch added the Defect label Jul 29, 2015

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Jul 29, 2015

Contributor

Something that might have been worth investigating is whether any of these are functionally never NULL, and in those cases whether passing a reference would be more appropriate.

Contributor

crabmusket commented Jul 29, 2015

Something that might have been worth investigating is whether any of these are functionally never NULL, and in those cases whether passing a reference would be more appropriate.

@Areloch

This comment has been minimized.

Show comment
Hide comment
@Areloch

Areloch Jul 29, 2015

Contributor

Aye, that'd probably be a good thing for a more in-depth cleanup.

That said, Az tested and noted that this is causing some weirdities with terrain rendering, so consider this NOT safe to merge in currently. I'll do more testing tomorrow and correct whatever's derping out.

Contributor

Areloch commented Jul 29, 2015

Aye, that'd probably be a good thing for a more in-depth cleanup.

That said, Az tested and noted that this is causing some weirdities with terrain rendering, so consider this NOT safe to merge in currently. I'll do more testing tomorrow and correct whatever's derping out.

@Azaezel

This comment has been minimized.

Show comment
Hide comment
@Azaezel

Azaezel Jul 29, 2015

Contributor

suggested revision after a few pokes:

   // Get the material if its a mesh.
   BaseMatInstance* matInst = NULL;
   if ( isMeshInst || isDecalMeshInst )
      matInst = static_cast<MeshRenderInst*>(inst)->matInst;

   if ( matInst )
   {
      // Skip decals if they don't have normal maps.
      if (isDecalMeshInst && !matInst->hasNormalMap())
         return;

      // If its a custom material and it refracts... skip it.
      if (matInst->isCustomMaterial() &&
         static_cast<CustomMaterial*>( matInst->getMaterial() )->mRefract )
         return;

      // Make sure we got a prepass material.      
      matInst = getPrePassMaterial( matInst );
      if ( !matInst || !matInst->isValid() )
         return;
   }
Contributor

Azaezel commented Jul 29, 2015

suggested revision after a few pokes:

   // Get the material if its a mesh.
   BaseMatInstance* matInst = NULL;
   if ( isMeshInst || isDecalMeshInst )
      matInst = static_cast<MeshRenderInst*>(inst)->matInst;

   if ( matInst )
   {
      // Skip decals if they don't have normal maps.
      if (isDecalMeshInst && !matInst->hasNormalMap())
         return;

      // If its a custom material and it refracts... skip it.
      if (matInst->isCustomMaterial() &&
         static_cast<CustomMaterial*>( matInst->getMaterial() )->mRefract )
         return;

      // Make sure we got a prepass material.      
      matInst = getPrePassMaterial( matInst );
      if ( !matInst || !matInst->isValid() )
         return;
   }
@Areloch

This comment has been minimized.

Show comment
Hide comment
@Areloch

Areloch Jul 29, 2015

Contributor

Alright, PR updated, not seeing any problems on my end.

Contributor

Areloch commented Jul 29, 2015

Alright, PR updated, not seeing any problems on my end.

Areloch added a commit that referenced this pull request Aug 23, 2015

Merge pull request #1378 from Areloch/PVS_Cleanup_595
Properly testing pointer vars aren't null before using them.

@Areloch Areloch merged commit b54f41e into GarageGames:development Aug 23, 2015

@Areloch Areloch deleted the Areloch:PVS_Cleanup_595 branch Jun 23, 2017

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