[PATCH] D53206: Allow padding checker to traverse simple class hierarchies

[Ben Craig via Phabricator via cfe-commits]

bcraig added a comment.

You should probably add whoever the current code owner of that static analyzer to this review.  I have very little involvement with Clang these days, so a "LGTM" from me doesn't carry much weight.



================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:155-157
+      if ((CXXRD->field_empty() && CXXRD->getNumBases() != 1) ||
+          CXXRD->getNumBases() != 0)
         return true;
----------------
1. correctness: If CXXRD->getNumBases() returns 1, this will return true, regardless of the emptiness.

2. style: split this into two if statements.  Everything else in this chunk of code _could_ be represented as one massive or-ed together conditional, but instead it uses multiple, back-to-back ifs.  This may not make sense after fixing the correctness bug.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:167-171
     }
+    // How do you reorder fields if you haven't got any?
+    else if (RD->field_empty())
+      return true;
+
----------------
I think this should just be an if, and not an else if.


================
Comment at: test/Analysis/padding_inherit.cpp:2-4
+
+// A class that has no fields and one base class should visit that base class
+// instead.
----------------
Evil test case to add...

```
struct Empty {};
struct DoubleEmpty : Empty {
    Empty e;
};
```

I don't think that should warn, as you can't reduce padding by rearranging element.

I guess your new code shouldn't catch that at all anyway, as you are testing in the other direction.... when field-less inherits from field-ed.



Repository:
  rC Clang

https://reviews.llvm.org/D53206