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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
