hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:563
+ for (auto &FixIt : FixIts) {
+ // Allow fixits within a single macro-arg expansion to be applied.
+ if (FixIt.RemoveRange.getBegin().isMacroID() &&
----------------
sammccall wrote:
> hokein wrote:
> > I feel a bit nervous about this (even it is for macro-arg expansion only),
> > as macro is very tricky.
> >
> > I think we may result in invalid code after applying the fixits in some
> > cases:
> >
> > 1) if the fix is to remove an unused variable (interestingly, clang doesn't
> > provide fixit to remove an unused variable, but for unused lambda capture,
> > it does)
> >
> > ```
> > #define LA(arg1, arg2) [arg1, arg2] { return arg2;}
> > void test1(int x, int y) {
> > LA(x, y); // after fixit, it would become LA(, y)? or LA(y)
> > }
> > ```
> >
> > 2) if the fix is to add some characters to the macro argument, e.g. adding
> > a dereference `*`, the semantic of the code after macro expansion maybe
> > changed.
> >
> > ```
> > void f1(int &a);
> > void f2(int *a) {
> > f1(a); // clang will emits a diagnostic with a fixit adding preceding a
> > `*` to a.
> > }
> > ```
> >
> > maybe we should be more conservative? just whitelist some diagnostics?
> > fixing typos seems pretty safe.
> your `test1` example doesn't trigger this case because the fix has to delete
> a comma that's provided by the macro body - this patch doesn't change
> behavior.
>
> To construct an example that follows this schema:
> ```
> struct S { S(int *x); };
> int *x;
> S s1(*x); // fixit -> S s1(x);
> #define CONCAT(a,b) a b
> S s2(CONCAT(*, x)); // fixit -> S s2(CONCAT(, x));
> ```
>
> The fixed code compiles fine and addresses the error in the expected way. It
> may not be *idiomatic*, but this is also a pathological case. I think it's at
> least as good to offer the fix in this case, and certainly it's not a good
> reason to drop support for others..
>
> ---
>
> > void f1(int &a);
>
> I can't follow this example, there are no macros?
> Why would the insertion change semantics?
>
> ---
>
> > maybe we should be more conservative? just whitelist some diagnostics?
> > fixing typos seems pretty safe.
>
> I think this leaves a lot of value on the table - we've been conservative so
> far.
> The problem with whitelists is they're incomplete and outdated (e.g. we have
> a whitelist for include fixer that's very incomplete, and I haven't managed
> to get around to fixing it, and neither has anyone else).
> So I think we should use this (or a blacklist) only if we can show this
> plausibly causes real problems.
>
> (To put this another way: by being too aggressive we'll get more feedback, by
> being more conservative we'll continue to get none)
>
> your test1 example doesn't trigger this case because the fix has to delete a
> comma that's provided by the macro body - this patch doesn't change behavior.
ah, you are right.
> I can't follow this example, there are no macros?
> Why would the insertion change semantics?
sorry, the example was incomplete, the case is like
```
int f1(int &a);
#define ABC(x) *x + f1(x);
void f2(int *a) {
ABC(a); // fixit -> ABC(*a), incorrect for the `*x` in macro body.
}
```
if the macro argument is being used in multiple places of the macro body, it
probably leads to problems. I suspect this is common in practice, we should not
allow fixit in this case.
> think this leaves a lot of value on the table - we've been conservative so
> far.
fair enough.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78338/new/
https://reviews.llvm.org/D78338
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
