[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍
Stephane Moore via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 4 17:57:02 PDT 2019
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
More information about the cfe-commits
mailing list