Report Unknown Doc-Comment Tags in Slice #3127
Conversation
| for (; i != lines.end(); ++i) | ||
| { | ||
| const string l = IceInternal::trim(*i); | ||
| string line; | ||
| if (parseCommentLine(l, paramTag, true, name, line)) | ||
| string lineText; |
There was a problem hiding this comment.
Renamed to avoid shadowing the line method which is defined on this class.
There was a problem hiding this comment.
I think that the predominant philosophy in compilers is that: "a badly formatted comment should never break your build".
That's not correct. Regular comments are ignored and as a result can't be malformed.
On the other hand, we should treat malformed doc-comments (bad syntax, unknown tags, misapplied tag, etc.) as errors, not warning. There is no sensible reason to tolerate them.
|
After thinking about it more. I think even a warning is too harsh.
It's important to remember that there's two purposes to doc-comments. It just so happens that the Slice parser is only aware of a very limited subset of these. But we do not want to restrict users to only using the very small subset of Doxygen tags that we map in the generated code. TL;DR Just because we the parser doesn't generate code for a doc-comment tag, does not make that tag invalid. |
|
Just to be clear, I still that this approach where collect all the unknown tags and just slap them at the end of doc comments isn't correct. If the Slice parser isn't aware of a tag, it should just ignore it and move on. Obviously, the tags that the parser is aware of, we should validate. |
|
After discussion, we've come to the following middle ground:
|
| @@ -0,0 +1,3 @@ | |||
| WarningInvalidComment.ice:10: warning: ignoring unknown doc tag '@remarks' in comment | |||
There was a problem hiding this comment.
Should we refer to these comments as "doc comment[s]"?
I find the diagnostic "ignored xxx in comment" odd as everything in plain comment should be ignored.
If I write:
// @foo in a plain comment
I don't anticipate any diagnostic.
| @@ -38,7 +38,8 @@ namespace Slice | |||
| { | |||
| All, | |||
| Deprecated, | |||
| InvalidMetadata | |||
| InvalidMetadata, | |||
| InvalidComment | |||
There was a problem hiding this comment.
Should be InvalidDocComment. A plain comment is always valid and ignored.
| /// Contains unrecognized tags. | ||
| StringList misc() const; | ||
| /// Targets of @see tags. | ||
| /// Targets of '@see' tags. |
There was a problem hiding this comment.
We should rename this class DocComment.
|
I agree with renaming |
This PR fixes #2088.
Now, the Slice parser rejects unknown doc-comment tags (including
@remarks), making it's situation impossible to hit.It also adds a check which rejects multi-line
@seetags, since these are disallowed and@seeshould always be of the form:@see IDENTIFIERand nothing else.When the parser encounters either of these cases, we discard (ignore) the offending line, and issue a warning for it.
I think that the predominant philosophy in compilers is that: "a badly formatted comment should never break your build".
Since, on a logical level, compilers should be ignoring all comments in your source file.
Since we currently have no validation of comments, I also added a new "InvalidComment" diagnostic category, which this PR uses.