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