-
Notifications
You must be signed in to change notification settings - Fork 442
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
Clarify expected behavior of --W*=diagnostic
option when diagnostic
can be an error
#4365
Comments
#4366 is a potential fix for this, but I am not confident that it is the "correct" fix. |
Disclaimer: I wasn't there when most of this was designed. This is my opinion.
That seems quite bad to me. We probably should not have the same string key for both.
This one was intentional when the info diagnostic kind was added recently. I think this was a correct decision.
I agree. I guess a lot of things will break if an error is ignored. I'm not sure if this was a deliberate decision or if it is just not checked (maybe @mihaibudiu or @ChrisDodd could know). I would suggest:
I'm not sure what could break if we changed this though. |
I'm largely in agreement with @vlstill - errors should not be demotable, but info and warning should be promotable. I think demoting (or suppressing) info's and warnings should be allowed - if you've acknowledged a warning and it's OK in your environment, then removing this to prevent it obscuring other, unknown/unexpected, warnings should be OK. (Other systems/tools permit this.) As a general point, I believe that we should make more use of the error_type for warnings/info so that individual messages can be suppressed, rather than classes or messages. Again, this is common with messaging utilities. |
As a side-effect, #4197 also disallows An alternate approach is #4366, which allows things such as If the side-effect I mentioned above was unintentional, then #4366 might be a correct fix for |
As a side note, the description for
|
@ajwalton @vlstill @fruffy @jafingerhut Let me know which approach you prefer and I will update #4366 accordingly. |
I think the diagnostic kind (e.g. |
Yes, this can be confusing. At the same time, applying
I can imagine doing what you did in #4366 plus adding a warning into a But ultimately I think we should try to avoid having the same diagnostic kind in multiple of the error/warning/info categories. |
You are right. I can add back the #4197 error only for |
…w `--Winfo=diagnostic` to work for `diagnostic`s that can be both warnings and errors. (#4366)
#4366 is merged, is this issue resolved? Or is there follow-up work planned? |
Nope, I have nothing else planned for this.
#4366 addressed my primary concern by not allowing errors to be demoted. But feel free to leave this open to address the remaining concern:
I'm not sure a consensus has been reached here. Depending on what the conclusion is, we may wish to either remove all such overlapping diagnostics, or just leave them as they are. |
@asl @vlstill @fruffy @ajwalton I have a new question related to this. Currently, the following options, for example:
Will promote all messages to errors, and therefore, the requested Is this the desired behavior? Or would it make more sense if |
IMO, explicit "disable" should always be honored. I guess for warnings we need to have a tri-state:
|
Do you mean even for message types that are originally errors? Or only for message types that are originally warning/info and get promoted to error via |
For only promoted ones. |
I agree that messages that were originally errors should never be demoted. The compiler might not generate valid output when an error is generated, so we can't allow these to be ignored. |
I agree. In summary a tri-state for warnings where There is also a way to wildcard-promote all infos to warnings ( |
Some warnings and errors have the same name, for example
ERR_INVALID
andWARN_INVALID
:As a result, for example,
--Wdisable=invalid
will disable errors of typeERR_INVALID
. Is this expected?I have the same question about the
--Wwarn
option.Also, the
--Winfo
option seems to explicitly disallowdiagnostic
s that are both errors and warnings, which is inconsistent with how--Wwarn
and--Wdisable
treats suchdiagnostic
s.My opinion is that allowing errors to be disabled is a bad idea, as disabling some error could result in unexpected behavior, but maybe there was a reason to allow this...
A few examples:
The text was updated successfully, but these errors were encountered: