erichkeane added inline comments.
================ Comment at: clang/test/Preprocessor/suggest-typoed-directive.c:3 + +// id: not suggested to '#if' +// ifd: expected-warning@+11 {{invalid preprocessing directive, did you mean '#if'?}} ---------------- aaron.ballman wrote: > This still suggests that something's wrong as I would imagine this would have > an edit distance of 1. Oh, interesting... setting the replacement option to > `false` may have made things better for the `elfindef` case but worse for the > `id` case? > > This is tricky because we want to identify things that are most likely simple > typos but exclude things that may reasonably not be a typo but a custom > preprocessor directive. Based on that, I *think* setting the replacement > option to `true` gives the more conservative answer (it treats a replacement > as 1 edit rather than 2). @erichkeane -- do you have thoughts? Not particularly. I don't have a good hold of how much we want to suggest with the 'did you mean'. Line 9 and line 10 here are unfortunate, I would hope those would happen? Its unfortunate we don't have a way to figure out these common typos. ================ Comment at: clang/test/Preprocessor/suggest-typoed-directive.c:10 +// expected-warning@+11 {{'#elfidef' directive not found, did you mean '#elifdef'?}} +// expected-warning@+11 {{'#elfindef' directive not found, did you mean '#elifdef'?}} +// expected-warning@+11 {{'#elsi' directive not found, did you mean '#else'?}} ---------------- aaron.ballman wrote: > ken-matsui wrote: > > aaron.ballman wrote: > > > ken-matsui wrote: > > > > aaron.ballman wrote: > > > > > ken-matsui wrote: > > > > > > aaron.ballman wrote: > > > > > > > It's interesting that this one suggested `#elifdef` instead of > > > > > > > `#elifndef` -- is there anything that can be done for that? > > > > > > > > > > > > > > Also, one somewhat interesting question is whether we want to > > > > > > > recommend `#elifdef` and `#elifndef` outside of C2x/C++2b mode. > > > > > > > Those directives only exist in the latest language standard, but > > > > > > > Clang supports them as a conforming extension in all language > > > > > > > modes. Given that this diagnostic is about typos, I think I'm > > > > > > > okay suggesting the directives even in older language modes. > > > > > > > That's as likely to be a correct suggestion as not, IMO. > > > > > > > It's interesting that this one suggested `#elifdef` instead of > > > > > > > `#elifndef` -- is there anything that can be done for that? > > > > > > > > > > > > I found I have to use `std::min_element` instead of > > > > > > `std::max_element` because we are finding the nearest (most minimum > > > > > > distance) string. Meanwhile, `#elfindef` has 2 distance with both > > > > > > `#elifdef` and `#elifndef`. After replacing `std::max_element` with > > > > > > `std::min_element`, I could suggest `#elifndef` from `#elfinndef`. > > > > > > > > > > > > > Also, one somewhat interesting question is whether we want to > > > > > > > recommend `#elifdef` and `#elifndef` outside of C2x/C++2b mode. > > > > > > > Those directives only exist in the latest language standard, but > > > > > > > Clang supports them as a conforming extension in all language > > > > > > > modes. Given that this diagnostic is about typos, I think I'm > > > > > > > okay suggesting the directives even in older language modes. > > > > > > > That's as likely to be a correct suggestion as not, IMO. > > > > > > > > > > > > I agree with you because Clang implements those directives, and the > > > > > > suggested code will also be compilable. I personally think only not > > > > > > compilable suggestions should be avoided. (Or, we might place > > > > > > additional info for outside of C2x/C++2b mode like `this is a > > > > > > C2x/C++2b feature but compilable on Clang`?) > > > > > > > > > > > > --- > > > > > > > > > > > > According to the algorithm of `std::min_element`, we only get an > > > > > > iterator of the first element even if there is another element that > > > > > > has the same distance. So, `#elfindef` only suggests `#elifdef` in > > > > > > accordance with the order of `Candidates`, and I don't think it is > > > > > > beautiful to depend on the order of candidates. I would say that we > > > > > > can suggest all the same distance like the following, but I'm not > > > > > > sure this is preferable: > > > > > > > > > > > > ``` > > > > > > #elfindef // diag: unknown directive, did you mean #elifdef or > > > > > > #elifndef? > > > > > > ``` > > > > > > > > > > > > I agree with you because Clang implements those directives, and the > > > > > > suggested code will also be compilable. I personally think only not > > > > > > compilable suggestions should be avoided. (Or, we might place > > > > > > additional info for outside of C2x/C++2b mode like this is a > > > > > > C2x/C++2b feature but compilable on Clang?) > > > > > > > > > > I may be changing my mind on this a bit. I now see we don't issue an > > > > > extension warning when using `#elifdef` or `#elifndef` in older > > > > > language modes. That means suggesting those will be correct but only > > > > > for Clang, and the user won't have any other diagnostics to tell them > > > > > about the portability issue. And those particular macros are > > > > > reasonably likely to be used in a header where the user is trying to > > > > > aim for portability. So I'm starting to think we should only suggest > > > > > those two in C2x mode (and we should probably add a portability > > > > > warning for #elifdef and #elifndef, so I filed: > > > > > https://github.com/llvm/llvm-project/issues/55306) > > > > > > > > > > > I would say that we can suggest all the same distance like the > > > > > > following, but I'm not sure this is preferable: > > > > > > > > > > The way we typically handle this is to attach FixIt hints to a note > > > > > instead of to the diagnostic. This way, automatic fixes aren't > > > > > applied (because there are multiple choices to pick from) but the > > > > > user is still able to apply whichever fix they want in an IDE or > > > > > other tool. It might be worth trying that approach (e.g., if there's > > > > > only one candidate, attach it to the warning, and if there are two or > > > > > more, emit a warning without a "did you mean" in it and use a new > > > > > note for the fixit. e.g., > > > > > ``` > > > > > #elfindef // expected-warning {{invalid preprocessing directive}} \ > > > > > expected-note {{did you mean '#elifdef'?}} \ > > > > > expected-note {{did you mean '#elifndef'?}} > > > > > ``` > > > > > WDYT? > > > > > I may be changing my mind on this a bit. I now see we don't issue an > > > > > extension warning when using `#elifdef` or `#elifndef` in older > > > > > language modes. That means suggesting those will be correct but only > > > > > for Clang, and the user won't have any other diagnostics to tell them > > > > > about the portability issue. And those particular macros are > > > > > reasonably likely to be used in a header where the user is trying to > > > > > aim for portability. So I'm starting to think we should only suggest > > > > > those two in C2x mode (and we should probably add a portability > > > > > warning for #elifdef and #elifndef, so I filed: > > > > > https://github.com/llvm/llvm-project/issues/55306) > > > > > > > > > > > > > Certainly, it would be less confusing to the user to avoid suggestions > > > > regarding those two. > > > > I'm going to fix my code to avoid suggesting them in not C2x mode. > > > > > > > > Thank you for submitting the issue, I also would like to work on it. > > > > > > > > > The way we typically handle this is to attach FixIt hints to a note > > > > > instead of to the diagnostic. This way, automatic fixes aren't > > > > > applied (because there are multiple choices to pick from) but the > > > > > user is still able to apply whichever fix they want in an IDE or > > > > > other tool. It might be worth trying that approach (e.g., if there's > > > > > only one candidate, attach it to the warning, and if there are two or > > > > > more, emit a warning without a "did you mean" in it and use a new > > > > > note for the fixit. e.g., > > > > > ``` > > > > > #elfindef // expected-warning {{invalid preprocessing directive}} \ > > > > > expected-note {{did you mean '#elifdef'?}} \ > > > > > expected-note {{did you mean '#elifndef'?}} > > > > > ``` > > > > > WDYT? > > > > > > > > This is really cool, but don't you care about the redundancy of `did > > > > you mean` in terms of the llvm team culture? > > > > If not, I will implement notes like the above. > > > > Certainly, it would be less confusing to the user to avoid suggestions > > > > regarding those two. I'm going to fix my code to avoid suggesting them > > > > in not C2x mode. > > > > > > +1, thank you! > > > > > > > This is really cool, but don't you care about the redundancy of did you > > > > mean in terms of the llvm team culture? If not, I will implement notes > > > > like the above. > > > > > > I would care if the list were potentially unbounded (like, say, with > > > identifiers in general), but because we know this list will only have a > > > max of two entries on it in this case, it seems reasonable to me. I > > > double-checked with @erichkeane to see if he thought it would be an > > > issue, and he agreed that it being a fixed list makes it pretty > > > reasonable. > > > > > > However, that discussion did raise a question -- why are there two > > > suggestions? elifdef requires a swap + delete and elifndef requires just > > > a swap, so we would have thought that it would have been the only option > > > in the list. > > With the implementation of Lev distances used in llvm, I could simply > > suggest `#elifdef` from `#elfidef` and `#elifndef` from `#elfindef`. > > > > So, in this situation, do you think that we still need to add two notes for > > conflicted distances? > No, let's skip the two note behavior. If we find ourselves with multiple > suggestions, we'll just leave off the "did you mean?" part of the diagnostic > entirely. Might I suggest we just emit the 'first' suggestion? This isn't really perfect, but I would think that our users put very little thought into it when we suggest the wrong thing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124726/new/ https://reviews.llvm.org/D124726 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits