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