bcraig marked 3 inline comments as done.
bcraig added a comment.
In http://reviews.llvm.org/D14779#309170, @zaks.anna wrote:
> With respect to the issues this checker found, I suggest submitting patches
> for the clang issues that can be fixed. Can the x-macro case be suppressed
> with the recommended suppression? I'd also submit a patch to gtest.
> Submitting patches with the fixes provides a good evaluation of new checkers:)
I will get some NFC changes for the easy fixes in the LLVM and Clang
repositories. The x-macro case is in compiler-rt.
The short answer to the x-macro suppression question is "no". The longer
answer is "yes, but...". Here are the first few relevant parts of the
compiler-rt x-macro...
COMMON_FLAG(
bool, symbolize, true,
"If set, use the online symbolizer from common sanitizer runtime to turn "
"virtual addresses to file/line locations.")
COMMON_FLAG(
const char *, external_symbolizer_path, 0,
"Path to external symbolizer. If empty, the tool will search $PATH for "
"the symbolizer.")
Nothing would break if I reorganized all the "COMMON_FLAG" calls such that the
const char * versions came before all the bool versions. It would cause these
two options to become more distant in both the code, and in the help. That
seems bad.
Alternatively, we could change the other side of the xmacro to generate an
anonymous union of TYPE and void *, and that would suppress the warning and
work correctly. However, that would increase the amount of dead-space in the
object. That seems counter to the goal of the checker.
If there were inline notations for suppressing warnings, then that would be the
best way to handle this case.
Note that this kind of issue comes up to a lesser degree in very large hand
written classes as well (i.e. not x-macro based). clang::Sema in
clang/include/clang/Sema/Sema.h is almost 9000 lines long, and a lot of the
members are grouped by usage instead of by alignment. Re-ordering those fields
would likely be detrimental to the readability of that class.
================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:194
@@ +193,3 @@
+ /// 3. If no fields can fit, pad by rounding the current offset up to our
+ /// lowest aligned field. Measure and track the amount of padding added.
+ /// Go back to 2.
----------------
zaks.anna wrote:
> It's not 100% clear what the "lowest aligned field" is.
Adjusting the comment. "Round up to at least the smallest field alignment that
we currently have"
http://reviews.llvm.org/D14779
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
