[clang-tidy][NFC] Simplify ClangTidyStats

[carlosgalvezp]

Summary

• Use NSDMI and remove constructor.

[salman-javed-nz]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

50

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.

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

50

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.

[whisperity]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

50

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.

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

50

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.

[aaron.ballman]

Btw, I'm in WG14 (C) standards meetings all week this week and on vacation next week, so my availability for code reviews is pretty limited for the next while. So despite my comments here, if you don't get a response back from me in a timely manner, don't hold up the review on my account if someone else approves (I have no blocking concerns with this patch).

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

50

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

61–64

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.)

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

50

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 :)

61–64

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 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.

[aaron.ballman]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

50

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.

61–64

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.

[carlosgalvezp]

• Use = initialization.
• Revert to unsigned.

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

50

:)

61–64

Thanks, I see how this change is not as NFC as I thought would be. Reverting!

[carlosgalvezp]

Fix refactoring bug.

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

61–64

But this seems more a style choice

PS: IMHO signed vs unsigned is not just a style choice. Using unsigned types for the wrong purpose (e.g. doing arithmetic) can lead to nasty logical bugs, especially when implicit conversions happen under the hood:

https://godbolt.org/z/5qox7b1nW

The above code is obvious, but in a real-world case you might not keep in the back of your head that you are operating with an "unsigned" variable in the middle of a 100-lines long function.

In the end, it's all about where you place the "singularity". With unsigned types, underflow happens around 0. With signed types, UB happens at min()/max(). In real-world applications, we usually code much more often around 0 than around min/max.

That's my 2 cents, don't mean to change/promote any guidelines. If you think this is a topic of interest I can bring it to the mailing list, otherwise I'll just drop it :)

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

61–64

That's my 2 cents, don't mean to change/promote any guidelines. If you think this is a topic of interest I can bring it to the mailing list, otherwise I'll just drop it :)

I have no idea if the community has an appetite for a change to the LLVM coding guidelines or not. However, if you feel strongly, you could certainly put an RFC up on llvm-dev and cfe-dev to see if there's support for some changes.

FWIW, I agree with you that using the wrong type can lead to logical bugs. I was mostly saying that the definition of "wrong type" is only clear in some circumstances rather than all. But the type choice has impacts on diagnostics, overload resolution, SFINAE, etc. which is why I'm skeptical we can have a blanket rule.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

 struct ClangTidyError : tooling::Diagnostic {
   ClangTidyError(StringRef CheckName, Level DiagLevel, StringRef BuildDirectory,
                  bool IsWarningAsError);
 
   bool IsWarningAsError;
   std::vector<std::string> EnabledDiagnosticAliases;
 };
 
-/// Contains displayed and ignored diagnostic counters for a ClangTidy
-/// run.
+/// Contains displayed and ignored diagnostic counters for a ClangTidy run.
 struct ClangTidyStats {
-  ClangTidyStats()
-      : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), ErrorsIgnoredNOLINT(0),
-        ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
-
-  unsigned ErrorsDisplayed;
-  unsigned ErrorsIgnoredCheckFilter;
-  unsigned ErrorsIgnoredNOLINT;
-  unsigned ErrorsIgnoredNonUserCode;
-  unsigned ErrorsIgnoredLineFilter;
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};
+  int ErrorsIgnoredNOLINT{0};
+  int ErrorsIgnoredNonUserCode{0};
+  int ErrorsIgnoredLineFilter{0};
 
-  unsigned errorsIgnored() const {
+  int errorsIgnored() const {
     return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
            ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }
 };
 
 /// Every \c ClangTidyCheck reports errors through a \c DiagnosticsEngine
 /// provided by this context.
 ///
 /// A \c ClangTidyCheck always has access to the active context to report
 /// warnings like:
 /// \code