Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Improve #error and #warning tokenization #251

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve #error and #warning tokenization #251

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b00fcb
:bug: #233
Oct 31, 2017
80b5cca
:bug: #233
Nov 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 45 additions & 6 deletions grammars/c.cson
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,23 +94,62 @@
]
}
{
'begin': '^\\s*((#)\\s*(error|warning))\\b'
'captures':
'begin': '^\\s*((#)\\s*(error|warning))\\b\\s*'
'beginCaptures':
'1':
'name': 'keyword.control.directive.diagnostic.$3.c'
'2':
'name': 'punctuation.definition.directive.c'
'end': '(?<!\\\\)(?=\\n)|(?=//)|(?=/\\*(?!.*\\\\\\s*\\n))'
'end': '(?<!\\\\)(?=\\n)'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the lookahead for comments was removed here. Was that intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short answer, yes. I see no benefit of having this, thus I removed it. I have to admit that it might not be the proper way to do things. While we have the opportunity, could you provide an example where having this makes a difference? I couldn't come up with one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I previously thought that #error test /* comment */ test would terminate the error message at the comment, but clearly based on your tests it doesn't.

'name': 'meta.preprocessor.diagnostic.c'
'patterns': [
{
'include': '#comments'
# double quoted string patterns for #error/warning lines (terminates at newline w/o line_continuation_character)
'begin': '"'
'beginCaptures':
'0':
'name': 'punctuation.definition.string.begin.c'
'end': '"|(?<!\\\\)(?=\\s*\\n)'
'endCaptures':
'0':
'name': 'punctuation.definition.string.end.c'
'name': 'string.quoted.double.c'
'patterns': [
{
'include': '#line_continuation_character'
}
]
}
{
'include': '#strings'
# single quoted string patterns for #error/warning lines (terminates at newline w/o line_continuation_character)
'begin': '\''
'beginCaptures':
'0':
'name': 'punctuation.definition.string.begin.c'
'end': '\'|(?<!\\\\)(?=\\s*\\n)'
'endCaptures':
'0':
'name': 'punctuation.definition.string.end.c'
'name': 'string.quoted.single.c'
'patterns': [
{
'include': '#line_continuation_character'
}
]
}
{
'include': '#line_continuation_character'
# unquoted strings patterns for #error/warning lines (terminates at newline w/o line_continuation_character)
'begin': '[^\'"]'
'end': '(?<!\\\\)(?=\\s*\\n)'
'name': 'string.unquoted.single.c'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just be string.unquoted.c

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name chose was based on already present string.quoted.single.c at line 133. Should I still change it to string.unquoted.c?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L133 is string.quoted.single.c because it uses single quotes. As an unquoted string (by the name) has no quotes, it should just be string.unquoted.c. This is consistent with other languages across the Atom organization.

'patterns': [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that GCC emits warnings by default if you have mismatched quotes, would it make sense to match for quotes here and scope them as invalid.deprecated.mismatched-quote.c?

{
  'match': '[\'"]'
  'name': 'invalid.deprecated.mismatched-quote.c'
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried your proposal and it looks like this:
image
I'm not sure what you means, English is not my mother tongue, I might have missed something.

My understanding is that GCC is not that cleaver. There is no warning about missing/missed used of quote for this:
#warning Well, you don't actually need quote, but it's highly recommended
but there is for:
#error doesn't

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 then let's not add it to avoid highlighting quotes as errors when they shouldn't be.

{
'include': '#line_continuation_character'
}
{
'include': '#comments'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this will match comments inside the unquoted error message such as #error test /* comment */ test.

Copy link
Author

@fnadeau fnadeau Jan 19, 2018 •
edited by winstliu
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, GCC output for
#error Unquoted message, /* yet another one */ but different
will be
issue_233.c:28:2: error: #error Unquoted message, but different
Comment is not actually part of the error message when unquoted.

If error message is quoted, then comment becomes part of it.

}
]
}
]
}
Expand Down