aaron.ballman 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;
----------------
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?


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

Reply via email to