[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:66-68
+  // FIXME: Express this as CharUnit rather than a hardcoded 8-bits (Rshift3)i
+  // After computing the minimum size in bits, check for an existing alignment
+  // flag.
----------------
I'm not certain I understand this FIXME, is it out of date? You're getting the width of a char and using that rather than hard-coding already.


================
Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:71
+  CharUnits MinByteSize =
+   //   CharUnits::fromQuantity(ceil((float)TotalBitSize / CHAR_BIT));
+      CharUnits::fromQuantity(ceil((float)TotalBitSize / CharSize));
----------------
This should probably be removed.


================
Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:85
+      ((Struct->getMaxAlignment() / CharSize) != NewAlign.getQuantity()) &&
+    //  ((Struct->getMaxAlignment() / CHAR_BIT) != NewAlign.getQuantity()) &&
+      (CurrSize != NewAlign) && !IsPacked) {
----------------
This should also probably be removed.


================
Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:93-94
+    diag(Struct->getLocation(),
+         "use \"__attribute__((packed))\" to reduce the amount of padding "
+         "applied to struct %0", DiagnosticIDs::Note)
+        << Struct;
----------------
Would it make sense to generate a fixit to add the attribute to the struct so long as the struct is not in a system header?


================
Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:107
+    diag(Struct->getLocation(),
+         "use \"__attribute__((aligned(%0)))\" to align struct %1 to %0 bytes",
+         DiagnosticIDs::Note)
----------------
Similar question here about the fixit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564