[PATCH] D147929: [clang-tidy] Fix handling of UseAssignment option in cppcoreguidelines-prefer-member-initializer

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added a subscriber: aaron.ballman.
carlosgalvezp added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp:134
+          Options.get("UseAssignment",
+                      OptionsView("modernize-use-default-member-init",
+                                  Context->getOptions().CheckOptions, Context)
----------------
PiotrZSL wrote:
> carlosgalvezp wrote:
> > This is very strange, feels like it's done to ensure the checks are in sync but IMO it creates more harm than good and makes the check harder to maintain. The checks are independent anyway (not aliases), so I believe it makes sense to keep being independent also in the options.
> > 
> > There are similar checks (e.g. magic numbers) where the user needs to either only enable one of the checks, or enter the same configuration settings twice.
> > 
> > I would vote for just treating this like an independent argument like all other checks, to avoid bugs like these.
> So remove dependency on modernize-use-default-member-init completly, or just on modernize-use-default-member-init options ?
I had a look at the original patch:

https://reviews.llvm.org/D71199

There's quite some discussion about the topic, and reviewers like @aaron.ballman weren't very happy about introducing this coupling. Seems like the problem is non-trivial so I vote for leaving your patch as is, which fixes a Github ticket and brings value, and think about completely removing the dependency to modernize-use-default-member-init on a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147929