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

[Alexander Kornienko via Phabricator via llvm-commits]

alexfh added a comment.

Thanks for the contribution and welcome to the LLVM community! A few more comments from me. I hope, this will help to tune to the LLVM coding style and conventions. This doc may be useful to read, if you haven't done so already: http://llvm.org/docs/CodingStandards.html



================
Comment at: clang-tidy/fpga/FPGATidyModule.cpp:5
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
----------------
Eugene.Zelenko wrote:
> License was changed this year. Same in other files.
Please mark the addressed comments "Done".


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:31
+  // If not a definition, do nothing
+  if (Struct != StructDef)
+    return;
----------------
If you need to only process struct definitions, it's better to use the `isDefinition()` matcher to limit the matches to definitions (`recordDecl(isStruct(), isDefinition())`).


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:37
+  unsigned int TotalBitSize = 0;
+  for (auto StructField : Struct->fields()) {
+    // For each StructField, record how big it is (in bits)
----------------
`const auto *` will make it clear that it's a pointer. Actually, I'd better specify the type explicitly.


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:45
+            .Width;
+    FieldSizes.push_back(std::pair<unsigned int, unsigned int>(
+        StructFieldWidth, StructField->getFieldIndex()));
----------------
`emplace_back(StructFieldWidth, StructField->getFieldIndex())` would be more succinct.


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:47
+        StructFieldWidth, StructField->getFieldIndex()));
+    // TODO: Recommend a reorganization of the struct (sort by StructField size,
+    // largest to smallest)
----------------
It's more common to use `FIXME` than `TODO` in comments in LLVM code.


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:55
+  CharUnits CurrSize = Result.Context->getASTRecordLayout(Struct).getSize();
+  CharUnits MinByteSize = CharUnits::fromQuantity((TotalBitSize + 7) >> 3);
+  CharUnits CurrAlign =
----------------
` / 8` would be clearer than ` >> 3` (and any sane compiler would optimize this itself: https://gcc.godbolt.org/z/nXdGeU, even though here the optimization doesn't bring anything).


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:63
+      NewAlign = NewAlign.alignTo(
+          CharUnits::fromQuantity(((int)NewAlign.getQuantity()) << 1));
+      // Abort if the computed alignment meets the maximum configured alignment
----------------
Too many parentheses here. And the `<< 1` would be easier to read as `* 2`, if there's no intentional behavior difference here.


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:65
+      // Abort if the computed alignment meets the maximum configured alignment
+      if (NewAlign.getQuantity() >= (1 << MAX_ALIGN_POWER_OF_TWO))
+        break;
----------------
A well named constant for `(1 << MAX_ALIGN_POWER_OF_TWO)` would make the code easier to understand, IMO.


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:75
+  if (Struct->hasAttrs()) {
+    for (auto StructAttribute : Struct->getAttrs()) {
+      if (std::string(StructAttribute->getSpelling()).compare("packed") == 0) {
----------------
Same comment about `auto` as above: please specify the type explicitly or at least use `const auto *`.


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:76
+    for (auto StructAttribute : Struct->getAttrs()) {
+      if (std::string(StructAttribute->getSpelling()).compare("packed") == 0) {
+        IsPacked = true;
----------------
It's better to construct an llvm::StringRef than a std::string: the former doesn't copy the contents. Next thing is that `operator==` should be preferred here, while `.compare()` is needed when ordering is important.

But here this all is irrelevant: `StructAttribute->getKind() == attr::Packed` allows to check this condition without comparing strings.


================
Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:100
+    diag(Struct->getLocation(),
+         "struct %0 has inefficient access due to poor alignment. Currently "
+         "aligned to %1 bytes, but size %3 bytes is large enough to benefit "
----------------
Diagnostic messages are not complete sentences. Thus, different punctuation is used: "alignment. Currently" -> "alignment; currently"


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

https://reviews.llvm.org/D66564