Details
Diff Detail
Event Timeline
Parse/ParseTemplate.cpp | ||
---|---|---|
492 | How does it work when you have typedef for the first template parameter? | |
Parser/typedef-instead-of-typename-typo.hpp | ||
3 | 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 | 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 | 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. |
Parse/ParseTemplate.cpp | ||
---|---|---|
492 | 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 | 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 | 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 | You are right. Thanks. |
Parse/ParseTemplate.cpp | ||
---|---|---|
492 | 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 | 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.
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. |
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.
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. |
How does it work when you have typedef for the first template parameter?