sammccall marked an inline comment as done.
sammccall 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() &&
----------------
hokein wrote:
> 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.
>
> 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.
This is definitely true.
As discussed offline, this is mitigated by:
- stringified expansions are common, but don't really count for this purpose
- fix will often make sense for all occurrences (we can't detect this), in
which case not offering it is **worse**
- remaining cases where fix makes sense for one case and not others probably
aren't that numerous
- behaviour in this case is to apply the fix and diagnose the resulting error
from the other expansion, which isn't that terrible (as far as macro diagnostic
experience goes)
Agreed to do it anyway and wait for feedback, I'll document this though.
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
