aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp:112
+ << Message->getMethodDecl()
+ << FixItHint::CreateReplacement(Message->getSourceRange(),
+ StringRef("[super init]"));
----------------
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];
}
```
================
Comment at: clang-tools-extra/test/clang-tidy/objc-super-self.m:41
+ INITIALIZER_IMPL();
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: suspicious invocation of 'self' in
initializer; did you mean to invoke a superclass initializer? [objc-super-self]
+}
----------------
stephanemoore wrote:
> aaron.ballman wrote:
> > Are you missing a `CHECK-FIXES` here?
> >
> > Personally, I don't think we should try to generate a fixit for this case,
> > so I would expect a CHECK-FIXES that ensures this doesn't get modified.
> No fix is currently generated for this case which is why there is no
> `CHECK-FIXES`. I also agree that no fix should be generated for this case.
>
> I must confess that I have yet to fully understand why the fix for this case
> is discarded (though I am grateful for the behavior). Let me dig around to
> try to better understand why no fixit is generated for this case and assess
> adding a condition for emitting the fixit.
Our typical way to check that a fix is not applied is to use `// CHECK-FIXES:
<original code>` to test that the fix was not applied.
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
