[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 added a comment. In D82728#2161152 , @xbolva00 wrote: > Is it possible to emit fixit note with "override" ? This is a good idea, though unfortunately (after eyeballing the implementation of `modernize-use-override` in clang-tidy

[PATCH] D82728: [clang] Add -Wsuggest-override

xbolva00 added a comment. Is it possible to emit fixit note with "override" ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82728/new/ https://reviews.llvm.org/D82728 ___ cfe-commits mailing list

[PATCH] D82728: [clang] Add -Wsuggest-override

This revision was automatically updated to reflect the committed changes. Closed by commit rG67895d47: [clang] Add -Wsuggest-override (authored by logan-5, committed by dblaikie). Changed prior to commit: https://reviews.llvm.org/D82728?vs=276248=277308#toc Repository: rG LLVM Github

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 added a comment. In D82728#2146067 , @dblaikie wrote: > Looks good - thanks for the patch and all the details! Might be worth turning > on by default in the LLVM build (after all the cleanup) Thanks a lot! I don't (think I) have commit access

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D82728#2142071 , @logan-5 wrote: > Feels like a dumb question, but I'm not sure if and how those build failures > are related to this patch?

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 added a comment. Feels like a dumb question, but I'm not sure if and how those build failures are related to this patch? They seem to involve completely separate parts of the LLVM project (and `.c` files to boot). Was it that the build was failing for an unrelated reason at the moment

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 marked an inline comment as done. logan-5 added inline comments. Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:1 +// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override + logan-5 wrote: > dblaikie wrote: >

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 marked an inline comment as done. logan-5 added inline comments. Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:1 +// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override + dblaikie wrote: > Does GCC have

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. Generally looks good to me (description/commit message should be updated now that the inconsistent inconsistency is no longer an issue) Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:1 +// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 updated this revision to Diff 276248. logan-5 added a comment. Fall back to -Wsuggest-override if -Winconsistent-missing-override is disabled. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82728/new/ https://reviews.llvm.org/D82728 Files:

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 added a comment. In D82728#2137492 , @dblaikie wrote: > Oh, yep, there's a way - it's usually used for performance-costly warnings, > to not spend the resources computing the warning if it's disabled anyway. Wow, thanks--overlooked that in a

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. In D82728#2137061 , @logan-5 wrote: > In D82728#2137021 , @dblaikie wrote: > > > I think it might be nice to make the -Wno-inconsistent-missing-override > > -Wsuggest-override situation a

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 updated this revision to Diff 276187. logan-5 added a comment. Addressed minor feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82728/new/ https://reviews.llvm.org/D82728 Files: clang/include/clang/Basic/DiagnosticGroups.td

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 marked 2 inline comments as done. logan-5 added a comment. In D82728#2137021 , @dblaikie wrote: > I think it might be nice to make the -Wno-inconsistent-missing-override > -Wsuggest-override situation a bit better (by having it still do the same

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. I'm generally on board with this, but would like @rsmith 's sign off to be sure. I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. In D82728#2136887 , @logan-5 wrote: > In D82728#2135149 , @dblaikie wrote: > > > Is the implementation you're proposing fairly consistent with GCC's? Run it > > over any big codebases to

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 updated this revision to Diff 276162. logan-5 added a comment. clang-formatted the diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82728/new/ https://reviews.llvm.org/D82728 Files: clang/include/clang/Basic/DiagnosticGroups.td

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 added a comment. In D82728#2135149 , @dblaikie wrote: > Is the implementation you're proposing fairly consistent with GCC's? Run it > over any big codebases to check it warns in the same places GCC does? This patch has the same behavior as

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. In D82728#2135142 , @logan-5 wrote: > Glad this is generating some discussion. For my $0.02, I would also > (obviously) love to be able to enable this warning on all the codebases I > work on, and this patch was born out of a

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 added a comment. Glad this is generating some discussion. For my $0.02, I would also (obviously) love to be able to enable this warning on all the codebases I work on, and this patch was born out of a discussion on the C++ Slack with another user who had found this warning very useful

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 updated this revision to Diff 275928. logan-5 marked 6 inline comments as done. logan-5 added a comment. Addressed some feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82728/new/ https://reviews.llvm.org/D82728 Files:

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. In D82728#2133951 , @Quuxplusone wrote: > In D82728#2133720 , @dblaikie wrote: > > > (the presence of at least one "override" being a signal the user intended > > to use override and

[PATCH] D82728: [clang] Add -Wsuggest-override

Quuxplusone added a comment. In D82728#2133720 , @dblaikie wrote: > (the presence of at least one "override" being a signal the user intended to > use override and missed some [...] I'm in favor of `-Wsuggest-override`, and would definitely use it if

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. Generally Clang tries to avoid stylistic warnings like this (& they are left to be addressed by tools like clang-tidy), hence why -Winconsistent-missing-override was implemented that way (the presence of at least one "override" being a signal the user intended to use

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 updated this revision to Diff 273991. logan-5 added a comment. Don't warn for destructors by default. This makes -Wsuggest-[destructor-]override more consistent with the behavior of -Winconsistent-missing-[destructor-]override, as well as gcc. Repository: rG LLVM Github Monorepo

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 updated this revision to Diff 273986. logan-5 added a comment. Ran clang-format over the diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82728/new/ https://reviews.llvm.org/D82728 Files: clang/include/clang/Basic/DiagnosticGroups.td

[PATCH] D82728: [clang] Add -Wsuggest-override

logan-5 created this revision. logan-5 added a reviewer: rsmith. logan-5 added a project: clang. Herald added a subscriber: cfe-commits. This patch adds `-Wsuggest-override`, which allows for more aggressive enforcement of modern C++ best practices, as well as better compatibility with gcc,