[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-      : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), ErrorsIgnoredNOLINT(0),
-        ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};
----------------
aaron.ballman wrote:
> carlosgalvezp wrote:
> > whisperity wrote:
> > > carlosgalvezp wrote:
> > > > salman-javed-nz wrote:
> > > > > What's the prevalent style for class member initialization? `=` or `{}`?
> > > > > cppcoreguidelines-prefer-member-initializer defaults to `{}` but I have seen both types in the code.
> > > > I tried to find this info in the LLVM coding guidelines but didn't find anything, so I assume it's maybe up to developers' discretion.
> > > > 
> > > > I prefer using braced initialization, since it prevents implicit conversions:
> > > > https://godbolt.org/z/4sP4rGsrY
> > > > 
> > > > More strict guidelines like Autosar enforce this. Also CppCoreGuidelines prefer that style as you point out.
> > > I think this is such a new and within LLVM relatively unused feature (remember we are still pegged to C++14...) that we do not have a consensus on style, and perhaps warrants discussing it on the mailing list.
> > Just to clarify - this is C++11/14 syntax, nothing really fancy. I'm happy to change to " = 0" if people want, I don't have a strong opinion here.
> I've seen both styles but I believe I've seen `=` used more often for scalar initialization and `{}` used more often for ctor initialization. e.g.,
> ```
> int a = 0; // More often
> int a{0}; // Less often
> 
> ClassFoo c = {1, 2, "three"}; // Less often
> ClassFoo c{1, 2, "three"}; // More often
> ```
Thanks for the info! IMO it's nice to have 1 way of initializing things consistently. Brace initialization is called "uniform initialization" for that reason - with the aim of uniforming the way of initializing data.

As said I have no strong opinion here, so I'll do what you guys tell me :) 


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:61-64
-  unsigned errorsIgnored() const {
+  int errorsIgnored() const {
     return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
            ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }
----------------
aaron.ballman wrote:
> The type changes overflow here from well-defined behavior to undefined behavior. I don't think that's a serious concern (we have error limits already), but in general, I don't think we should be changing types like this without stronger rationale. This particular bit feels like churn without benefit -- what do we gain by introducing the possibility for UB here? (I'm not strongly opposed, but I get worried when well-defined code turns into potentially undefined code in the name of an NFC cleanup.)
Do we have coding guidelines for when to use signed vs unsigned? I have quite ugly past experiences using unsigned types for arithmetic and as "counters" so these kinds of things really catch my eye quickly. This goes in line with e.g. the [[ https://google.github.io/styleguide/cppguide.html#Integer_Types | Google style guide ]].

Yes, you are correct that signed int overflow is UB, but would that then be an argument for banning signed ints in the codebase at all? IMO what's more important is to know the domain in which this is being applied. Do we expect people to have more than 2^31 linter warnings? If we do, would 2^32 really solve the problem, or just delay it? Maybe the proper solution is to go to 64-bit ints if that's an expected use case.

Anyway if this is a concern I'm happy to revert and take the discussion outside the patch. We used to be over-defensive in this topic as well (UB is not to be taken lightly) but realized that perhaps the problem lies somewhere else.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113847/new/

https://reviews.llvm.org/D113847