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

Mon Nov 15 05:47:38 PST 2021

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:
> > 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 :) 
> I usually refer to it as "unicorn initialization" because there's not nearly as uniform as people might expect. :-D I don't have strong preferences here though.
:) 

================
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:
> carlosgalvezp wrote:
> > 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.
> > Do we have coding guidelines for when to use signed vs unsigned?
> 
> We don't, and I don't think we could even if we wanted to. There are times when unsigned is correct and there are times when signed is correct, and no blanket rule covers all the cases.
> 
> > 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? 
> 
> No. My point about the change to UB wasn't "I think this is dangerous" -- as I mentioned, we have limits on the number of errors that can be emitted, so I don't believe we can hit the UB in practice. It's more that changing types is rarely an NFC change without further validation and it wasn't clear if this validation had happened. Changing types is a nontrivial (and rarely NFC) change in C++.
> 
> > 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.
> 
> Personally, I'm not super motivated to see a change here, but I'm also not opposed to it. It mostly just feels like churn. If we're solving a problem, I'm happy to see the changes, but we should call out what problem we're solving in the patch summary. But this seems more a style choice and I certainly wouldn't want to see these changes used as a precedent to go switch more unsigned ints to be signed ints in the name of style.
Thanks, I see how this change is not as NFC as I thought would be. Reverting!

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

https://reviews.llvm.org/D113847