LegalizeAdulthood marked 3 inline comments as done. LegalizeAdulthood added inline comments.
================ Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11 +`ES.31 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions>`_, and +`ES.32 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es32-use-all_caps-for-all-macro-names>`_. + ---------------- carlosgalvezp wrote: > LegalizeAdulthood wrote: > > carlosgalvezp wrote: > > > Is ES.32 really checked by this check? I don't see any example or test > > > that indicates that. > > > > > > I'm also unsure if ES.32 should be handled here or via the > > > "readability-identifier-naming" check, which is where you enforce a > > > particular naming convention for different identifiers. Setting ALL_CAPS > > > for macros there would be an effective way of solving ES.32. > > It was always handled through an option on this check. > > (Look at lines 49-56 of `MacroUsageCheck.cpp`) > > > > It's a little bit odd, because it either checks for the names > > or it checks for the constant/function like macros, it never > > does both at the same time. > > > > This is the way the check was originally written, I haven't > > changed any of that. > Ah I see it now! I got really confused by the `CheckCapsOnly` option. Not for > this patch, but I think the following could be improved: > > * Set it default to True, not False. People expect that check enforce a given > guideline as good as possible by default. Options exist to deviate from the > guidelines and relax them, which would be the case e.g. when introducing the > check in an old codebase. > > * Be renamed to something more descriptive and split it into 2 options with > one single purpose. Right now this option controls a) enforcing ES.32 and b) > applying warnings to macros with all caps or not. Yeah, I wasn't a fan of the way this option was influencing the behavior of this check. Can you open a github issue for the points you raised? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116386/new/ https://reviews.llvm.org/D116386 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
