ostannard added inline comments.
================ Comment at: clang/include/clang/Basic/CodeGenOptions.def:396 +/// according to the field declaring type width. +CODEGENOPT(ForceNoAAPCSBitfieldWidth, 1, 0) + ---------------- dnsampaio wrote: > ostannard wrote: > > Why is this a negative option, when the one above is positive? > The enforcing of number of accesses would not be accepted if it was not an > opt-in option. This one I expect it should be accepted with a single opt-out > option. My problem is with the name of the option (adding an extra negative just makes things more confusing), not with the default value. This could just be called `AAPCSBitfieldWidth`, (unless you think the `Force` is adding something), and default to true father than false. ================ Comment at: clang/include/clang/Driver/Options.td:2328 HelpText<"Follows the AAPCS standard that all volatile bit-field write generates at least one load. (ARM only).">; +def ForceNoAAPCSBitfieldWidth : Flag<["-"], "fno-AAPCSBitfieldWidth">, Group<m_arm_Features_Group>, + Flags<[DriverOption,CC1Option]>, ---------------- ostannard wrote: > Command-line options are in kebab-case, so this should be something like > `fno-aapcs-bitfield-width`. This also applies to the `fAAPCSBitfieldLoad` > option above, assuming it's not too late to change that. > > Please also add a positive version of this option (i.e. > `faapcs-bitfield-width`). This still needs a positive version. ================ Comment at: clang/lib/CodeGen/CGRecordLayout.h:83 + /// The offset within a contiguous run of bitfields that are represented as + /// a single "field" within the LLVM struct type. This offset is in bits. ---------------- These doc comments are copied from the ones above, they need changing. ================ Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:279 + lowerUnion(); + return computeVolatileBitfields(); + } ---------------- Returning `void` is confusing (yes I know it was already there), this should be a separate `return;` statement. ================ Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:288 + appendPaddingBytes(Size); + return computeVolatileBitfields(); + } ---------------- Same here. ================ Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:535 +/// +/// Enforcing the width restriction can be disable using +/// -fno-aapcs-bitfield-width. ---------------- s/disable/disabled/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
