[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

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