Remove WEXPERIMENTAL for "if with unsupported statements" #332
Conversation
|
Verification #14013436: ✅ pass |
| kind of statement as long as the `#if` condition doesn't reference | ||
| any identifiers other than `dml_1_2`, `true`, and `false`. | ||
| """ | ||
| fmt = "object of type %s not allowed inside `#if`" |
There was a problem hiding this comment.
%s declarations are not allowed inside #if
There was a problem hiding this comment.
Or maybe %s statements are not ...? Note that this is a best-effort error message, will e.g. say "extern_typedef statements are ..." which is not optimal but good enough. The important ones are param, import and template, and we get those right at least.
There was a problem hiding this comment.
How come you prefer statements instead of declarations? Sure, we have a concept of "object statements", but as you point out, many of the forbidden things doesn't fit inside that concept; and we have "declaration" as a collective name for object statements and global declarations (which sometimes is not ideal; for example in our documentation we often say "param declarations" as a collective name for all param object statements, which I dislike, but that's what we do. If we say "param declarations not allowed inside #if" I don't think people would get confused.)
There was a problem hiding this comment.
Also, this error message should be tagged version = "1.4". Toplevel if logic can only happen in 1.4 code.
There was a problem hiding this comment.
I just realized. The emission of this error message disagrees with non-toplevel ifs. They crop up from invocations of validate_if_body() from dmlparse.py; and those use ECONDP, ECONDT and ECONDINEACH. We'll have to address that; replace them all with EBADCONDSTMT?
There was a problem hiding this comment.
I don't see a contradiction here. The messages for non-top-level ifs do not need to mention that top-level ifs have a special case. They are also more specific than top-level ifs, and I don't see a problem with that. I would probably not oppose unifying the messages, but that can be done in a separate PR.
There was a problem hiding this comment.
The messages for non-top-level ifs do not need to mention that top-level ifs have a special case.
Well, no, I'm not arguing that (we've settled that a time ago.) I'm confused why you're bringing that up; what are you addressing with that statement? Or are you talking about documentation -- "the documentation for error messages of non-top-level ifs do not need to mention that top-level ifs have a special case, thus it's a good thing they're separate error classes?" I don't buy that argument at all.
They are also more specific than top-level ifs, and I don't see a problem with that.
Neither do I. What I have a problem with is that your PR introduces an inconsistency about what error classes are used for reporting the same kind of error (from the user's perspective.) Because of that, I don't buy the argument that unification can be done in a separate PR.
Thus I'm arguing either you replace ECONDP, ECONDT, and ECONDINEACH all with EBADCONDSTMT, or you replace EBADCONDSTMT with uses of ECONDP, ECONDT, and ECONDINEACH. Either way, you can implement it by having ast builder pass what forbidden declarations are in use, and then you can either report ECONDP/ECONDT/ECONDINEACH for each of them; or you can pass the sites and declaration kinds to EBADCONDSTMT to each be printed via print_site_message.
There was a problem hiding this comment.
What I have a problem with is that your PR introduces an inconsistency
It's not an inconsistency toward the end user. The same kind of mistake will always give an error, and in some circumstances it will use a somewhat different wording (essentially because there's more error cases to cover). The end result is that the user will read the message and adjust their code, and then they will not see the message again. What's the problem? The error tags are mainly related to our internal testing.
Hm, what we could do is to pick a more directed message, like "not allowed inside if, except dml_1_2" if the violating statement is one that only can appear on top level, like a typedef or template. And use a plain ECONDP etc for the other cases. Not sure if that makes anyone happier, though.
| to contain any type of statement, as long as the condition doesn't reference | ||
| any identifiers other than `dml_1_2`, `true` and `false`. This is often useful | ||
| while migrating the devices of a system from DML 1.2 to DML 1.4, | ||
| as it allows conditional definitions of templates in common code used from both DML 1.2 and DML 1.4. For example, let's say an existing template `reset_to_seven` |
There was a problem hiding this comment.
line surpasses 79 char width limit
There was a problem hiding this comment.
I don't think the same style rule applies for .md; IIRC @aconradi suggested "new line between sentences" as an alternative convention to make diffs more manageable.
For markdown embedded in source comments, the host language's conventions take precedence of course.
There was a problem hiding this comment.
Whichever criteria you choose, you fail it. If nothing else, "For example," should be on a new line.
I still vote for breaking on spaces except when ` is involved, because apparently the GH wiki inteprets line breaks within tick literally (which we need to clean up...)
The reason I vote for it is consistency with the rest of the document, as well as a slight preference that the boundary is consistent throughout the file (as opposed to natural linebreaks, since sentences will be of differing lengths.)
| @@ -196,3 +196,11 @@ | |||
| various operations that rely on the `register_view` interface, such as the | |||
| `dev_util.bank_regs` function and the `write-device-reg` and `probe-address` | |||
| CLI commands. | |||
| - `note 6` DMLC no longer emits warnings saying | |||
There was a problem hiding this comment.
will no longer emit the subset of
WEXPERIMENTALwarnings saying...
maybe?
There was a problem hiding this comment.
I chose not to because I didn't want to bore the reader with details, but it does make sense to mention WEXPERIMENTAL because of how --no-warn=WEXPERIMENTAL is sprinkled in makefiles.
There was a problem hiding this comment.
Yeah that's mostly why I figured it was a good idea.
RELEASENOTES.md
Outdated
| `param` and `template` are forbidden inside `#if`, but a special exception | ||
| allows forbidden statements to appear specifically inside an `#if (dml_1_2)` | ||
| block. The warning message was meant to highlight this irregularity, but | ||
| caused more harm than good. |
There was a problem hiding this comment.
more harm than good; error messages surrounding the special case have been improved instead.
| @@ -196,3 +196,11 @@ | |||
| various operations that rely on the `register_view` interface, such as the | |||
| `dev_util.bank_regs` function and the `write-device-reg` and `probe-address` | |||
| CLI commands. | |||
| - `note 6` DMLC no longer emits warnings saying | |||
There was a problem hiding this comment.
Toplevel if logic is 1.4 exclusive, and thus so should this releasenote.
There was a problem hiding this comment.
I disagree. You are right that they are exclusive for 1.4 files, but their only use is in the context of 1.2 devices: The only allowed identifier is dml_1_2, so the construct is only relevant for common code intended for use in 1.2.
There was a problem hiding this comment.
so the construct is only relevant for common code intended for use in 1.2.
Only for the time being. I think we've both agreed upon extending the conditional to also allow the use of the compat params?
Nevertheless, I guess this is a matter of perspective of where a releasenote belongs. I was thinking "1.4, because this feature is only available in 1.4 files", while you're arguing the use of it is only significant due to how it impact 1.2 devices. Because of that, I'm fine with it being in RELEASENOTES.dml; but I would note that your argument sounds like it'd belong in RELEASENOTES-1.2.md, which I would heavily disagree with.
There was a problem hiding this comment.
The releasenote split between 1.2/1.4/both is inherently fuzzy and somewhat questionable. What matters the most is that the end-user sees a note attached to the right package version, this happens no matter what releasenote file we pick; the choice of file is not so important.
I guess the advantage of the split is that many syntax additions are 1.4 specific, and putting it in -1.4 eliminates the need to mention this in text.
I think it would make sense at least to merge RELEASENOTES and RELEASENOTES-1.2. A total 7 notes were 1.2 specific over the past 3 years.
mandolaerik
force-pushed
the
pr/remove-wexperimental-for-if-with-unsupported-statements
branch
from
e5f4b1b to
cd00065
Compare
|
Verification #14117325: ✅ pass |
Remove WEXPERIMENTAL for "if with unsupported statements"