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

[Jan Korous via Phabricator via cfe-commits]

jkorous-apple 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);
----------------
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?


================
Comment at: Parser/typedef-instead-of-typename-typo.hpp:3
+
+template <typename A, typedef B> struct Foo {
+  a
----------------
vsapsai wrote:
> Maybe put this test in clang/test/FixIt ?
> 
> Also please check what file extensions are used for testing templates. .hpp reflects real-life usage but  most tests are .cpp. Or maybe I wasn't paying attention.
Oh, I completely missed that directory! Thanks.

It seems like there are some header files with ".h" extension but these are usually somehow special (empty file or something) so I figure you are right and the extension should be ".cpp".


================
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'?}} \
----------------
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?




================
Comment at: clang/Basic/DiagnosticParseKinds.td:1167
+def note_meant_to_use_typename : Note<
+  "Did you mean to use 'typename'?">;
 }
----------------
vsapsai wrote:
> Looks like other diagnostic messages "did you mean to use …" have lowercase "d" in "did". Though I haven't checked how it looks in various situations.
You are right. Thanks.


https://reviews.llvm.org/D42170