This is an archive of the discontinued LLVM Phabricator instance.

Fixit for 'typedef' instead of 'typename' typo
ClosedPublic

Authored by jkorous-apple on Jan 17 2018, 5:56 AM.

Diff Detail

Repository
rC Clang

Event Timeline

jkorous-apple created this revision.Jan 17 2018, 5:56 AM
jkorous-apple added a reviewer: arphaman.
vsapsai added inline comments.
Parse/ParseTemplate.cpp
492 ↗(On Diff #130152)

How does it work when you have typedef for the first template parameter?

Parser/typedef-instead-of-typename-typo.hpp
3 ↗(On Diff #130152)

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.

5 ↗(On Diff #130152)

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.

clang/Basic/DiagnosticParseKinds.td
1167 ↗(On Diff #130152)

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.

jkorous-apple added inline comments.Jan 23 2018, 4:11 AM
Parse/ParseTemplate.cpp
492 ↗(On Diff #130152)

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?

Parser/typedef-instead-of-typename-typo.hpp
3 ↗(On Diff #130152)

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".

5 ↗(On Diff #130152)

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?

clang/Basic/DiagnosticParseKinds.td
1167 ↗(On Diff #130152)

You are right. Thanks.

Changes based on Volodymyr's comments.

vsapsai added inline comments.Jan 23 2018, 11:31 AM
Parse/ParseTemplate.cpp
492 ↗(On Diff #130152)

I was suspicious that

if (isStartOfTemplateTypeParameter())
  return ParseTypeParameter(Depth, Position);

earlier in the function won't let your code run for the first template parameter. But isStartOfTemplateTypeParameter has

if (Tok.isNot(tok::kw_typename))
  return false;

so everything works fine.

Parser/typedef-instead-of-typename-typo.hpp
5 ↗(On Diff #130152)

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@-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

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.

I got rid of backslashes in test and added member of the type parameter with typo. Thanks for these suggestions.

I got rid of backslashes in test and added member of the type parameter with typo. Thanks for suggestions.

jkorous-apple edited reviewers, added: vsapsai; removed: arphaman.Jan 27 2018, 1:38 PM
vsapsai added inline comments.Jan 30 2018, 3:54 PM
FixIt/fixit-typedef-instead-of-typename-typo.cpp
14 ↗(On Diff #131693)

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@-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

template <typename A, typedef B> // expected-error{{expected template parameter}}
                                 // expected-note@-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.

Tried to make test more readable.

Thanks for your patience with me still learning lit/filecheck. What do you think about the test now? I totally agree that this is subjective matter but I appreciate your feedback.

vsapsai accepted this revision.Jan 31 2018, 9:53 AM

One small note about -fdiagnostics-parseable-fixits. The rest looks good to me. And the test looks readable.

FixIt/fixit-typedef-instead-of-typename-typo.cpp
1 ↗(On Diff #132135)

I don't think you need -fdiagnostics-parseable-fixits here. Though it is required in the second RUN line.

This revision is now accepted and ready to land.Jan 31 2018, 9:53 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.