Static Code Analysis With PVS-Studio (Part 4)

You know, I never expected to be writing up a 4th instalment of the PVS-Studio series so soon.. but here we are. A new version of PVS-Studio was released – and some new problems were found.

Here we go…

V519 The ‘bNoPenetrationDepth’ variable is assigned values twice successively. Perhaps this is a mistake.

As you can probably guess, it is indeed a bug. Here’s the code, from FHitResult::NetSerialize(), in Collision.cpp:-

uint8 Flags = (bBlockingHit << 0) | (bStartPenetrating << 1) | (bImpactPointEqualsLocation << 2) | (bImpactNormalEqualsNormal << 3) | (bInvalidItem << 4) | (bInvalidFaceIndex << 5) | (bInvalidFaceIndex << 6);
Ar.SerializeBits(&Flags, 7); 
bBlockingHit = (Flags & (1 << 0)) ? 1 : 0;
bStartPenetrating = (Flags & (1 << 1)) ? 1 : 0;
bImpactPointEqualsLocation = (Flags & (1 << 2)) ? 1 : 0;
bImpactNormalEqualsNormal = (Flags & (1 << 3)) ? 1 : 0;
bInvalidItem = (Flags & (1 << 4)) ? 1 : 0;
bInvalidFaceIndex = (Flags & (1 << 5)) ? 1 : 0;
bNoPenetrationDepth = (Flags & (1 << 6)) ? 1 : 0;

Nothing obviously wrong with bNoPenetrationDepth itself here … until you look at the first line. As it happens, that should be:-

uint8 Flags = (bBlockingHit << 0) | (bStartPenetrating << 1) | (bImpactPointEqualsLocation << 2) | (bImpactNormalEqualsNormal << 3) | (bInvalidItem << 4) | (bInvalidFaceIndex << 5) | (bNoPenetrationDepth << 6);

Oops…

V519 The ‘SplitterArea’ variable is assigned values twice successively. Perhaps this is a mistake.

This comes from the Construct() function for SAdvancedDropdownRow. The code:-

TSharedPtr<SWidget> SplitterArea;
if( bShowSplitter )
{
  SplitterArea = 
    SNew( SSplitter )
    .PhysicalSplitterHandleSize( 1.0f )
<-- LOTS MORE SETUP CODE -->
}
else
{
  SplitterArea = SNew(SSpacer);
}
SplitterArea = SNew(SSpacer);

Obviously, that last line shouldn’t be there and can be safely deleted.

Grab this change from Github here: github.com/EpicGames/UnrealEngine/pull/3726

V571 Recurring check. This condition was already verified in line 185.

Another copy-paste mistake, as it turns out:-

HLODOutliner::FDragValidationInfo HLODOutliner::FLODActorDropTarget::ValidateDrop(FDragDropPayload& DraggedObjects) const
{
  if (DraggedObjects.StaticMeshActors.IsSet() && DraggedObjects.StaticMeshActors->Num() > 0)
  {
    if (DraggedObjects.StaticMeshActors->Num() > 0 && DraggedObjects.LODActors->Num() == 0)

Apart from that we’re checking the number of static mesh actors twice, note the definition of LODActors:-

TOptional<TArray<TWeakObjectPtr<AActor>>> LODActors;

So we can see that the code should actually be:-

HLODOutliner::FDragValidationInfo HLODOutliner::FLODActorDropTarget::ValidateDrop(FDragDropPayload& DraggedObjects) const
{
  if (DraggedObjects.StaticMeshActors.IsSet() && DraggedObjects.StaticMeshActors->Num() > 0)
  {
    if (DraggedObjects.LODActors.IsSet() && DraggedObjects.LODActors->Num() == 0)
    {

(ie. in this case, we didn’t just find that a condition was being verified twice, we found a genuine problem in the code)

And finally…

V519 The ‘Visibility’ variable is assigned values twice successively. Perhaps this is a mistake.

Here’s the code:-

EVisibility SPropertyEditorColor::GetVisibilityForOpaqueDisplay() const
{
  // If the color has a non-opaque alpha, we want to display half of the box as Opaque so you can always see
  // the color even if it's mostly/entirely transparent.  But if the color is rendered as completely opaque,
  // we might as well collapse the extra opaque display rather than drawing two separate boxes.
  EVisibility Visibility = EVisibility::Collapsed;
  if( !bIgnoreAlpha )
  {
    const bool bColorIsAlreadyOpaque = (OnGetColor().A == 1.0);
    if (bColorIsAlreadyOpaque)
    {
      Visibility = EVisibility::Collapsed;
    }
    Visibility = EVisibility::Visible;
  }
  return Visibility;
}

You can see here that Visibility is being set to EVisibility::Collapsed under some conditions – but then always overridden with EVisibility::Visible. I’m not 100% sure what the intent is here, actually, so I’ll bottle out and leave this one as an exercise for the reader.

Anyway, that’s enough for this time! Back soon – hopefully with something a little meatier than these small code fixes

Credit(s): Robert Troughton (Coconut Lizard),
Evgeniy Ryzhkov (Viva64)
Status: Currently unfixed in 4.12