carlosgalvezp added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23
-namespace {
-
-bool isCapsOnly(StringRef Name) {
- return std::all_of(Name.begin(), Name.end(), [](const char C) {
- if (std::isupper(C) || std::isdigit(C) || C == '_')
- return true;
- return false;
+static inline bool isCapsOnly(StringRef Name) {
+ return llvm::all_of(Name, [](const char C) {
----------------
LegalizeAdulthood wrote:
> carlosgalvezp wrote:
> > Nit: `inline` can be removed.
> Yeah, my IDE flagged it but since you asked for the `static` .... `:)`
Sorry for the confusion, I should have added inline fixes directly to make it
clear :)
================
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>`_.
+
----------------
LegalizeAdulthood wrote:
> 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?
Absolutely, much better place for this!
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp:1
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t --
-header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later
%t -- -header-filter=.* -system-headers --
----------------
LegalizeAdulthood wrote:
> carlosgalvezp wrote:
> > I'm curious as to why this is needed. If I remove it the test fails, on
> > line 15, but the `u8` prefix was introduced already since C++11?
> The `u''` (UTF-16) and `U''` (UTF-32) character literals were added in C++11.
> The `u8''` (UTF-8) character literal was added in C++17.
> https://en.cppreference.com/w/cpp/language/character_literal
Duh, I was checking //string// literal, not //character// literal
https://en.cppreference.com/w/cpp/language/string_literal
Repository:
rG LLVM Github Monorepo
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
