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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: FixIt/fixit-typedef-instead-of-typename-typo.cpp:14
+// CHECK: expected-error at -6 {{unknown type name 'a'}}
+// CHECK: expected-error at -6 {{expected member name or ';' after declaration specifiers}}
----------------
You don't need `// CHECK:` prefix for `expected-error`. And without `FileCheck` CHECK lines aren't verified. In fact, the test should fail as it expects the fix-it to be for the line 4, not line 3. I'm not sure you can test diagnostic and parseable fixits with a single RUN line, luckily we can have multiple RUN lines.

I don't think those relative error lines are good from readability standpoint. But please keep in mind that all my comments about readability are subjective and should be taken with the grain of salt. The problem is that one needs to count lines to understand which line diagnostic is referring to. From my experience it is easy to understand something like
```
blah  // expected-error
      // expected-note at -1
```
because you don't need to calculate lines, it is clear -1 refers to the previous line. But in your case I have to calculate 6 or 7 lines back.

Here is for comparison "expected-" annotations closer to corresponding lines
```lang=c++
template <typename A, typedef B> // expected-error{{expected template parameter}}
                                 // expected-note at -1{{did you mean to use 'typename'?}}
// CHECK: fix-it:{{.*}}:{4:23-4:30}:"typename"
struct Foo {
  // Should not produce error about type since parsing speculatively with fixit applied.
  B member;
  // Let's just check that other errors get reported.
  a // expected-error{{unknown type name 'a'}}
}; // expected-error{{expected member name or ';' after declaration specifiers}}
```
I'm not entirely happy with the result as it looks too dense and noisy but it conveys my thought.


https://reviews.llvm.org/D42170