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