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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
