[PATCH] D42170: Fixit for 'typedef' instead of 'typename' typo

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: Parse/ParseTemplate.cpp:492
+  // Is there just a typo in the input code? ('typedef' instead of 'typename')
+  if (Tok.is(tok::kw_typedef)) {
+    Diag(Tok.getLocation(), diag::err_expected_template_parameter);
----------------
jkorous-apple wrote:
> vsapsai wrote:
> > How does it work when you have `typedef` for the first template parameter?
> It works all right but I am puzzled by your question now :-)
> 
> 
> ```
> /Users/jankorous/src/oss/llvm/llvm/tools/clang/test/Parser/typedef-instead-of-typename-typo.hpp:3:11: error: expected template parameter
> template <typedef A, typename B> struct Foo {
>           ^
> /Users/jankorous/src/oss/llvm/llvm/tools/clang/test/Parser/typedef-instead-of-typename-typo.hpp:3:11: note: Did you mean to use 'typename'?
> template <typedef A, typename B> struct Foo {
>           ^~~~~~~
>           typename
> fix-it:"/Users/jankorous/src/oss/llvm/llvm/tools/clang/test/Parser/typedef-instead-of-typename-typo.hpp":{3:11-3:18}:"typename"
> /Users/jankorous/src/oss/llvm/llvm/tools/clang/test/Parser/typedef-instead-of-typename-typo.hpp:4:3: error: unknown type name 'a'
>   a
>   ^
> 
> ```
> 
> I am probably not following you here. Do you have any specific reason on your mind? Anything I should think about or check in the source code?
I was suspicious that
```lang=c++
  if (isStartOfTemplateTypeParameter())
    return ParseTypeParameter(Depth, Position);
```
earlier in the function won't let your code run for the first template parameter. But `isStartOfTemplateTypeParameter` has
```lang=c++
  if (Tok.isNot(tok::kw_typename))
    return false;
```
so everything works fine.


================
Comment at: Parser/typedef-instead-of-typename-typo.hpp:5
+  a
+}; // CHECK: expected-error{{expected template parameter}} \
+// CHECK: expected-note{{Did you mean to use 'typename'?}} \
----------------
jkorous-apple wrote:
> vsapsai wrote:
> > It is a little bit confusing to what lines the messages would be attributed to. Need to check locally because not sure I interpret all those backslashes the same way lit does.
> > 
> > Also idea for the test. To check that the fix-it was applied properly you can add a member like `B b;` and it shouldn't trigger any errors.
> I am totally open to suggestions how to write better tests using FileCheck. As far as I understand it I have to keep the expected-* macro on the same line as the code. Or is there any better way?
> 
> Do I understand it right that you suggest to create another test in which to try applying fixit and check that compilation was successful? Do you mean to create compile_commands.json temporarily and use clang-check -fixit?
> 
> 
To answer your question. You can keep the expected-* macro on the same line and attach subsequent expectations with backslash. Another option is doing something like `expected-note at -1 {{...}}`. That `-1` is a relative line number.

Back to the test case. I believe you need to separate expected-* checks from fixit output check. The former uses `-verify` and the latter `| FileCheck %s`. I think test/FixIt/fixit-vexing-parse.cpp is a good example. And remember that you can split long lines, `template <typename A, typedef B> struct Foo` doesn't have to be on the same line.

For suggested test I didn't mean anything fancy. Just something like adding a line `B b;` Because currently for
```lang=c++
template <typename A, typedef B> 
struct Foo {
    B b;
};
```
the output is
```
recovery.cc:1:31: error: unknown type name 'B'
template <typename A, typedef B>
                              ^
recovery.cc:3:5: error: unknown type name 'B'
    B b;
    ^
2 errors generated.
```
And with your change we won't have the second error.


https://reviews.llvm.org/D42170