[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

Sockke created this revision. Sockke added reviewers: aaron.ballman, MTC, steven.zhang. Herald added subscribers: shchenz, kbarton, xazax.hun, nemanjai. Sockke requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. In C++, the enumerat

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

whisperity added a comment. Is this the right decision to make, conceptually? It will leave the variable uninitialised still, and reading such an uninit variable is still an issue, even if it is an enum. Could we consider the alternative of warning the user about the uninitialized variable, jus

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

steven.zhang added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables.cpp:103 + // Expect no warning given here. + Color color; + // Expect no warning given here. Technical speaking, we should warn about s

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

MTC added a comment. In D106431#2892859 , @whisperity wrote: > Is this the right decision to make, conceptually? It will leave the variable > uninitialised still, and reading such an uninit variable is still an issue, > even if it is an enum. Yeah, th

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

MTC added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables.cpp:103 + // Expect no warning given here. + Color color; + // Expect no warning given here. steven.zhang wrote: > Technical speaking, we should

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

steven.zhang added a comment. In D106431#2892866 , @MTC wrote: > In D106431#2892859 , @whisperity > wrote: > >> Is this the right decision to make, conceptually? It will leave the variable >> uninitialised still

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

Sockke added a comment. At present, Our views on preventing UB are basically the same, so a warning still needs to be reported (BTW, the original version will not report warnings for enum class types). The final question is what is the recommended value for initialization and whether to provide

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

whisperity added a comment. The problem with enums is that translating //zero// (0, 0.0, nullptr, etc...) to the enum case is not always apparent. A warning **should** always be given. And //if// you can find a zero member in the enum, we can report an automated suggestion for that. To give an

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

aaron.ballman added a comment. In D106431#2896206 , @whisperity wrote: > The problem with enums is that translating //zero// (0, 0.0, nullptr, etc...) > to the enum case is not always apparent. A warning **should** always be > given. And //if// you can

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

whisperity added a comment. In D106431#2896334 , @aaron.ballman wrote: > In D106431#2896206 , @whisperity > wrote: > >> The problem with enums is that translating //zero// (0, 0.0, nullptr, >> etc...) to the en

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

aaron.ballman added a comment. In D106431#2896367 , @whisperity wrote: > In D106431#2896334 , @aaron.ballman > wrote: > >> In D106431#2896206 , @whisperity >> wrote: >>

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

whisperity added a comment. In D106431#2896441 , @aaron.ballman wrote: > However, I don't recall how clang-tidy interacts with fix-its on notes off > the top of my head, so I'm making an assumption that clang-tidy's automatic > fixit applying mode hand

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

aaron.ballman added a comment. In D106431#2896472 , @whisperity wrote: > In D106431#2896441 , @aaron.ballman > wrote: > >> However, I don't recall how clang-tidy interacts with fix-its on notes off >> the top o

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

Sockke added a comment. In D106431#2896472 , @whisperity wrote: > In D106431#2896441 , @aaron.ballman > wrote: > >> However, I don't recall how clang-tidy interacts with fix-its on notes off >> the top of my he

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

Sockke added a comment. Any thoughts? : ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106431/new/ https://reviews.llvm.org/D106431 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

whisperity added a comment. In D106431#2907002 , @Sockke wrote: > Any thoughts? : ) First, let's first fix that we should still warn for the uninitialised `enum` case, without a FixIt. That's the issue at hand, right now, Clang-Tidy generates, as you

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

steven.zhang added a comment. In D106431#2907688 , @whisperity wrote: > In D106431#2907002 , @Sockke wrote: > >> Any thoughts? : ) > > First, let's first fix that we should still warn for the uninitialised `enum