-
Notifications
You must be signed in to change notification settings - Fork 259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding a new CLI option for deprecation; changing deprecation of OldSemi #4041
Conversation
@@ -31,6 +31,10 @@ public class CommonOptionBag { | |||
"Print additional information such as which files are emitted where.") { | |||
}; | |||
|
|||
public static readonly Option<bool> WarnDeprecation = new("--warn-deprecation", () => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a general mechanism for turning off warnings, and once we have that options like these become obsolete and should be removed, so maybe it's better not to add them in the first place.
I think the ErrorId
concept you added could be used for turning off warnings.
…to cok-semicolons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the addition of /deprecation:0
in those files when there are non-semi-colon deprecation warnings. Instead, for those files, please remove the offending semi-colons.
@@ -1,4 +1,4 @@ | |||
// RUN: %dafny /compile:0 /autoTriggers:0 "%s" > "%t" | |||
// RUN: %dafny /compile:0 /deprecation:0 /autoTriggers:0 "%s" > "%t" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is /deprecation:0
added in lieu of removing the semi-colons that are being warned about? If so, then I suggest instead spending the time to remove the deprecated semi-colons now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see now. It's because there are so many such files. Okay, I'm fine with those being changed in a future PR. But for now, I suggest reverting the addition ot /deprecation:0
in those cases when adding that flag removes the previous warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are quite a few files with quite a few semicolon errors. However there were only 5 files that had changes in the expected result by adding /deprecation:0 (because they suppressed other warnings). I did the work of removing semicolons from 4 of them and the /deprecation:0, and there now there is no change to expected result.
The 5th is server/symbols.transcript
, which I do not have the tools to edit.
And there are ~100 files remaining with /deprecation:0
Partially addresses #4037
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.