steveire added inline comments.
================ Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:118 + /// behavior of clang-tidy. + virtual llvm::Optional<ast_type_traits::TraversalKind> + getCheckTraversalKind() const; ---------------- aaron.ballman wrote: > steveire wrote: > > aaron.ballman wrote: > > > steveire wrote: > > > > sammccall wrote: > > > > > I don't really get why this would be optional. > > > > > A check's implementation isn't really going to be robust to "whatever > > > > > the default is", it's going to be tested against one or the other. > > > > > So None isn't really a sensible return value - can't the base class > > > > > simply return the actual default? > > > > ASTMatchFinder doesn't know the ASTContext, so it can't access the > > > > default. That's why I made it an optional. > > > The fact that it's optional is a bit weird. Given that this is a > > > temporary API, I see two approaches: hold our nose because the ugliness > > > shouldn't be long-lived, or have the default be specified in two places > > > with comments saying "don't forget to update over here if you change this > > > value" so we can get rid of the `Optional<>` here. WDYT? > > My preference is to keep the optional. Having multiple places to update > > makes it likely they'll get out of sync. > Then I think it's fine to leave it as `Optional<>` given that it's a > temporary API for easing the transition. @sammccall, are you okay with that? > > In terms of eventually removing this API, what's your plan? I noticed we > can't do something nice like slapping a `[[deprecated]]` attribute on the > declaration and have overriders get a notification that the method they're > overriding is deprecated. Will we just pull the API entirely and force users > to react to the code breakage? > Will we just pull the API entirely and force users to react to the code > breakage? Yes, I think something like that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80623/new/ https://reviews.llvm.org/D80623 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
