[PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

Ben Craig via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 11 06:28:57 PDT 2016

bcraig added a comment.

Note: In the future, try to provide more context lines in the patch.  http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

I'm mostly fine with the algorithmic changes.  I'm not thrilled with the formatting of the error message, but I'm not sure if much can be done about it without causing tons of work.

Here's my wishlist items...

1. List each field on an individual line, preferably with the type mentioned as well.
2. Get the field ordering information into a note or fixit.
3. Provide a more stable ("stable" as in stable sort) field ordering.  I've got an inline comment describing what I mean with that.

I'm not sure if my first two items are possible without large overhauls.  The third isn't a big deal either.

So to recap, I'm strongly in favor of the idea.  I'm weakly in favor of the implementation.  I'll give you a shot to polish things a bit if you have the inclination.  If you don't, then I won't let my idea of a perfect (and expensive) implementation stand in the way of this good implementation.


More information about the cfe-commits mailing list