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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I, too, don't believe this is FPGA specific; it should likely go into `misc-` or even `performance-`.
The wording of the diags seems weird to me, it would be good to 1. add more explanation to the docs and 2. reword the diags.



================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:31
+  // Get sizing info for the struct
+  std::vector<std::pair<unsigned int, unsigned int>> FieldSizes;
+  unsigned int TotalBitSize = 0;
----------------
`llvm::SmallVector`


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:50
+  CharUnits CurrSize = Result.Context->getASTRecordLayout(Struct).getSize();
+  CharUnits MinByteSize = CharUnits::fromQuantity((TotalBitSize + 7) / 8);
+  CharUnits CurrAlign =
----------------
`roundUpTo()`?


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:54-62
+  if (!MinByteSize.isPowerOfTwo()) {
+    int MSB = (int)MinByteSize.getQuantity();
+    for (; MSB > 0; MSB /= 2) {
+      NewAlign = NewAlign.alignTo(
+          CharUnits::fromQuantity(((int)NewAlign.getQuantity()) * 2));
+      // Abort if the computed alignment meets the maximum configured alignment
+      if (NewAlign.getQuantity() >= MAX_CONFIGURED_ALIGNMENT)
----------------
I'm not sure what is going on here.
I think this should be a helper function with meaningful name.


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:67-76
+  // Check if struct has a "packed" attribute
+  bool IsPacked = false;
+  if (Struct->hasAttrs()) {
+    for (const Attr *StructAttribute : Struct->getAttrs()) {
+      if (StructAttribute->getKind() == attr::Packed) {
+        IsPacked = true;
+        break;
----------------
```
bool IsPacked = Struct->hasAttr<attr::Packed>();
```


================
Comment at: docs/clang-tidy/checks/fpga-struct-pack-align.rst:45
+
+  // Explicitly aligning a struct to a bad value will result in a warning
+  struct badly_aligned_example {
----------------
'bad'?


================
Comment at: test/clang-tidy/fpga-struct-pack-align.cpp:9
+};
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: struct 'error' has inefficient access due to padding, only needs 10 bytes but is using 24 bytes, use "__attribute((packed))" [fpga-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: warning: struct 'error' has inefficient access due to poor alignment; currently aligned to 8 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [fpga-struct-pack-align]
----------------
'has inefficient access' reads weird to me. I'm not sure what that actually means.


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

https://reviews.llvm.org/D66564