[PATCH] D124726: Suggest typoed directives in preprocessor conditionals

[Ken Matsui via Phabricator via llvm-commits]

ken-matsui added inline comments.


================
Comment at: clang/test/Preprocessor/suggest-typoed-directive.c:10
+// expected-warning at +11 {{'#elfidef' directive not found, did you mean '#elifdef'?}}
+// expected-warning at +11 {{'#elfindef' directive not found, did you mean '#elifdef'?}}
+// expected-warning at +11 {{'#elsi' directive not found, did you mean '#else'?}}
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > 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.  
> Notionally that makes sense, but the situation I'm worried about is when users pass `-fixit` to the compiler -- then it automatically does whatever, but we have no idea whether that whatever is reasonable or not when we have more than one option.
I could suggest `#if` from `#id`, but I had to change `#elfindef` to `#elfinndef` to suggest `#elifndef`.
After setting the `AllowReplacements` option to `true`, `#elfindef` suggested `#elifdef` (I think it might have the same distance with `#elifndef`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124726/new/

https://reviews.llvm.org/D124726