This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy][NFC] Simplify ClangTidyStats
ClosedPublic

Authored by carlosgalvezp on Nov 14 2021, 9:18 AM.

Details

Summary
  • Use NSDMI and remove constructor.

Diff Detail

Event Timeline

carlosgalvezp created this revision.Nov 14 2021, 9:18 AM
carlosgalvezp requested review of this revision.Nov 14 2021, 9:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2021, 9:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
salman-javed-nz added inline comments.
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 added inline comments.Nov 14 2021, 11:20 PM
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 added inline comments.Nov 15 2021, 1:17 AM
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 added inline comments.Nov 15 2021, 1:27 AM
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.

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 added inline comments.Nov 15 2021, 5:06 AM
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 added inline comments.Nov 15 2021, 5:26 AM
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 edited the summary of this revision. (Show Details)
  • Use = initialization.
  • Revert to unsigned.
carlosgalvezp added inline comments.Nov 15 2021, 5:47 AM
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!

Fix refactoring bug.

carlosgalvezp added inline comments.Nov 15 2021, 9:50 AM
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 accepted this revision.Nov 15 2021, 10:46 AM

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.

This revision is now accepted and ready to land.Nov 15 2021, 10:46 AM
This revision was automatically updated to reflect the committed changes.