Skip to content
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

VHDL: Add private, view keywords; Distinguish attribute from keyword #3389

Merged
merged 2 commits into from
Mar 14, 2022

Conversation

hoonweiting
Copy link
Contributor

I'm not familiar with the language, but based on the file history, I presume that any changes to the language after 2015 are not reflected. There's been at least one new version since then (VHDL-2019, some highlights summarised on another site), and brings at least two new keywords: private and view. I'd confirm with the language reference manual but it costs like $500 and it's not quite worth the cost for what I'll use it for :P

I've also separated the attributes from the keywords, and used a generic pattern instead of a name list for the attributes (similar to Ada).

@github-actions
Copy link

github-actions bot commented Mar 13, 2022

JS File Size Changes (gzipped)

A total of 1 files have changed, with a combined diff of -122 B (-15.4%).

file master pull size diff % diff
components/prism-vhdl.min.js 794 B 672 B -122 B -15.4%

Generated by 🚫 dangerJS against 6eab90a

@RunDevelopment
Copy link
Member

The coverage test failing isn't your fault.

That test uses a few different strategies to determine whether a pattern has been covered. The most basic strategy is to simply check that a regex produced some token. However, it uses a different strategy for keyword lists. It requires all keyword in the list to procure a token.

The test now fails because you simplified the keyword enough for the coverage test to recognize it as a keyword list, and now it wants you to test all of them.

I'm sorry about that, but could you please add the missing keywords to the test?

but it costs like $500

lol. Yeah, that's not quite worth it.

I've also separated the attributes from the keywords, and used a generic pattern instead of a name list for the attributes

Sounds good.

@hoonweiting
Copy link
Contributor Author

Ooh that's interesting! I'll update the tests shortly. My apologies for not running all the tests locally!

@hoonweiting
Copy link
Contributor Author

Ah hmm... I did not realise that library and use are also contained the in keyword list, I thought they were only considered constants.

I'll update the keyword test, but I'm not sure whether to remove constant, or allow library and use to be assigned both constant and keyword, as they are both standard tokens. I'm leaning towards removing constant, but I wonder why those two are constants as well. I might have to look for whatever VHDL docs I can find about it.

@RunDevelopment
Copy link
Member

Ooh that's interesting! I'll update the tests shortly. My apologies for not running all the tests locally!

It's not your fault. The coverage check isn't part of the test suite (because it doesn't test the behavior and correctness of anything), so it's hard to find.

library and use

I think they're both keyword. The spec from 2000 describes a library clause (section 11.2, page 153) and a use clause (section 10.4, page 150). The example from page 11 also highlights them as keywords:

image

@hoonweiting
Copy link
Contributor Author

@RunDevelopment I see! Thanks for searching that up :)

I ran the test on my laptop, it timed out the first time, but all of it passed after plugging my laptop in.

@RunDevelopment RunDevelopment merged commit d1a5ce3 into PrismJS:master Mar 14, 2022
@RunDevelopment
Copy link
Member

Thank you!

I ran the test on my laptop, it timed out the first time

Do you remember which one timed out?

@hoonweiting
Copy link
Contributor Author

Ah no, not really. I just tried to run it again after unplugging it, but it hasn't failed again yet.

I tried to google some of the words I remember seeing in the error, this question on StackOverflow seems to match it.

As for the file, I believe it was coverage.js, which makes sense because I only ran npm run regex-coverage.

@hoonweiting hoonweiting deleted the 3170-vhdl-attribute branch March 14, 2022 15:56
@RunDevelopment
Copy link
Member

A timeout on coverage? Huh, that's strange. The test just runs all languages tests, so it doesn't do any heavy computations. Anyway, I'll keep that in mind for the future.

@hoonweiting
Copy link
Contributor Author

It's probably just me not plugging in my laptop. For example, if I load a game without plugging it in first, it'll be really, really laggy. I also notice this lag when running commands from the terminal. It's probably fine normally!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants