[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore planned changes to this revision.
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp:112
+      << Message->getMethodDecl()
+      << FixItHint::CreateReplacement(Message->getSourceRange(),
+                                      StringRef("[super init]"));
----------------
aaron.ballman wrote:
> stephanemoore wrote:
> > aaron.ballman wrote:
> > > stephanemoore wrote:
> > > > stephanemoore wrote:
> > > > > aaron.ballman wrote:
> > > > > > This could be dangerous if the `[super self]` construct is in a macro, couldn't it? e.g.,
> > > > > > ```
> > > > > > #define DERP self
> > > > > > 
> > > > > > [super DERP];
> > > > > > ```
> > > > > Good point. Let me add some test cases and make sure this is handled properly.
> > > > Added some test cases where `[super self]` is expanded from macros.
> > > You missed the test case I was worried about -- where the macro is mixed into the expression. I don't think we want to try to add a fix-it in that case.
> > Added a test case though at the moment it generates a fixit.
> > 
> > Before I investigate modifying the check to avoid generating a fixit in this case, I think it would be helpful for me to understand your concerns in that scenario better. Is your concern that a proper fix might involve fixing the macro itself rather than the message expression?
> > 
> > ```
> > #if FLAG
> > #define INVOCATION self
> > #else
> > #define INVOCATION init
> > #endif
> > 
> > - (instancetype)init {
> >   return [super INVOCATION];
> > }
> > ```
> > Before I investigate modifying the check to avoid generating a fixit in this case, I think it would be helpful for me to understand your concerns in that scenario better. Is your concern that a proper fix might involve fixing the macro itself rather than the message expression?
> 
> Essentially, yes. Macros can get arbitrarily complex and so our rule of thumb is to not apply fix-its when the code being fixed is within a macro. Another example that can be tricky is adding another layer of macros:
> ```
> #define FOO super
> #define BAR self
> #define BAZ FOO BAR
> 
> - (instancetype)init {
>   return [BAZ];
> }
> ```
Thanks for the explanation! I'll get started making the changes 👍


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59806