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:
> 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)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78338/new/
https://reviews.llvm.org/D78338
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
