[SR-226] Deprecation of C-style for loops #552
Conversation
|
Please add tests. |
|
Added tests for the fix-its, cleaned up all the existing tests that use c-style for. Fixed positioning of the first replace. All set, I hope. |
gregomni
changed the title
| @@ -800,6 +800,9 @@ class ForStmt : public LabeledStmt { | |||
|
|
|||
| SourceLoc getStartLoc() const { return getLabelLocOrKeywordLoc(ForLoc); } | |||
| SourceLoc getEndLoc() const { return Body->getEndLoc(); } | |||
|
|
|||
| SourceLoc getSemicolon1Loc() const { return Semi1Loc; } | |||
| SourceLoc getSemicolon2Loc() const { return Semi2Loc; } | |||
There was a problem hiding this comment.
Can we use "getFirstSemicolonLoc()" and "getSecondSemicolonLoc()" here?
|
Thanks for working on this! Some comments above. |
|
Aha. I had missed the wrinkle that getEndLoc() points to the start of the last token. That and the idiomatic use of getLocForEndOfToken() makes this vastly easier, and I don't specifically need the "=" location in a PatternBindingDecl after all. I think I've cleaned up all the bits you had feedback on @DougGregor, thanks! |
Warns of deprecation, checks all the appropriate bits to see if we can do an automatic fix, and generates fix-its if that is valid. Also adds a note if the loop looks like it ought to be a simple for-each, but really isn’t because the loop var is modified inside the loop.
|
Noticed there was a conflict (silly one where I added an expected error in a test next to another commit that changed a ++ to a +1). So rebased/merged/squashed. Hopefully merge-able to base now? |
|
This looks great! |
[SR-226] Deprecation of C-style for loops
|
Wow, this is pretty sweet, I'm impressed with your work on the fixit too. Please add at least one testcase for the fixit rewrite. You can use syntax like: expected-error {{...}} {{1-2=}} -verify mode will then barf and tell you what to put into the {{1-2=}} section. -Chris |
|
Oh, nevermind, I see you got a few, thanks again! |
|
Actually, I have a question regarding the fixits: is there a way to test for the absence of specific fixits? Several of the added tests could really use an "expect-no-fixits" equivalent. |
|
No, we don't have a way to do that right now. One thing I toyed with at one point was to make -verify mode require that fixits are specified in the test. If this were enabled, then we could express this. You can take a look at the code to do this in DiagnosticVerifier.cpp. Another interesting mode (when changing diagnostics) is the "autoApplyFixes = false;" line at the end of that file. Flip it and the compiler rewrites testcases to match what the compiler is currently producing :-) |
[Stress tester XFails] Update XFails
Warns of deprecation, checks all the appropriate bits to see if we can do an automatic fix, and generates fix-its if that is valid.
Also adds a note if the loop looks like it ought to be a simple for-each, but really isn’t because the loop var is modified inside the loop.
I do have one outstanding FIXIT that the first replace range can be wrong. And, of course, there are lots of tests and stdlib places to fix to get rid of the warnings this’ll produce.
I chose to do the diagnostics here after type checking instead of early in the parser because while most of this would be more easily done in the parser, the check for modifying the loop var in the body would be Very Difficult. Would love feedback. Especially if there is a cleaner way to do all the AST tree pattern matching required here (all the if dyn_casts).