[lmi] Using clang-tidy for detecting reserved identifiers and more (was: [lmi-commits] master 15b60953 7/8: Avoid reserved names and anything resembling them)

[Vadim Zeitlin]

On Thu, 19 May 2022 20:25:21 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 5/19/22 18:09, Vadim Zeitlin wrote:
GC> > 
GC> >  Please consider integrating clang-tidy rather than trying to (poorly)
GC> > reproduce (some of) its functionality. I can easily do this for GitHub CI
GC> > builds, of course, but I don't think this is going to be sufficient. I do
GC> > think that using it is worth the trouble, as it could be used to detect
GC> > this and tons of other potential problems.
GC> I really just wanted to permit '__func__' and maybe '__builtin_',
GC> but I saw a "TODO ??" marker and decided to follow it down the
GC> rabbit hole, which went deeper than I'd hoped. The result isn't
GC> pretty, so I separated the commits for easy reversion in the
GC> "golden future time".
GC> 
GC> My hesitation is that the clang rabbit hole may be deeper still,
GC> and I could get lost there for months. More rewarding, certainly,
GC> but deeper. If you want to blaze the trail in CI, then maybe I'll
GC> be able to follow.

 I've tried using clang-tidy and it does work nicely without any particular
problems (although it's not fast, as it basically does the work of the
entire compiler front-end and then some), but it requires some changes to
the sources to suppress the warnings that we don't want to see because,
AFAICS, it doesn't have any way to define suppressions outside of the code.

 E.g. running "clang-tidy -checks='-*,bugprone-reserved-identifier'"
without any special options results in (tons of) warnings like this:

config.hpp:104:16: warning: declaration uses identifier '_ISOC99_SOURCE', which 
is a reserved identifier [bugprone-reserved-identifier]

This can be suppressed with

---------------------------------- >8 --------------------------------------
diff --git a/config.hpp b/config.hpp
index dec1d737a..117839b86 100644
--- a/config.hpp
+++ b/config.hpp
@@ -101,6 +101,7 @@

 #if defined __GNUC__
 #   if !defined _ISOC99_SOURCE
+        // NOLINTNEXTLINE(bugprone-reserved-identifier)
 #       define _ISOC99_SOURCE
 #   endif // !defined _ISOC99_SOURCE
 #endif // defined __GNUC__
---------------------------------- >8 --------------------------------------

(there is also just "// NOLINT" which can be added to the end of the line,
but NOLINTNEXTLINE seems cleaner in this particular case) but the question
is whether you're ready to uglify the sources like this just to satisfy the
linter. Of course, it's also possible that we can avoid this particular
warning in a better way by just stopping defining _ISOC99_SOURCE, as I'm
not sure at all it's (still) needed -- at least simply removing it, as done
in the head commit of xanadu/remove-ISOC99_SOURCE-def branch (which also
contains the other commits from PR 210 to make the CI build pass with
clang) doesn't break any tests.

 Anyhow, my question is whether I should try to find some other way to
suppress the warnings about _ISOC99_SOURCE in the CI build using clang-tidy
if we want add one, or if we can fix it in lmi itself, either by
suppressing it with the special linter comment or, maybe, by just getting
rid of this definition entirely.

 Also, more generally, do you think these special comments are acceptable,
should we decide to enable more checks, resulting in some false positives?

 Thanks,
VZ

pgpLYe321tMCS.pgp
Description: PGP signature