Closed Sv3n closed 6 years ago
I am out of town today until towards the end of the week. I'll review the PR when I get home. Thanks for the work!
I'm back in the office and I'll review. I noted that you marked it work in progress so I won't merge just yet. I'll let you know if I have any feedback.
Okay, actually that was pretty short, so here's my comments.
1) Good job on navigating that syntax file! That is not an easy thing to do. I tried to make it pretty common-sense for names and to create robust "objects" for inclusion but it's not at all a simple task.
2) I was gratified to see you picked up the changes in the index and listing for the ST find features. In the back of my head when I was having shower thoughts about your changes as described (but not in detail) I figured that might need to be update with new scope names for procedures and functions, but you nailed it.
3) I don't have any strong feelings for procedures and functions as scoped. You are correct that there are not 1-to-1 correlations between the scope naming conventions and the VHDL language. I seem to remember back when I was writing it debating in my head which way I should go. Especially with the newer release expanding and improving on color schemes, your solution of picking up the entity.name.function
portion and then using a subscope to distinguish procedure versus function is arguably the best option.
4) I have SOME purity concerns about scoping processes as functions however I recognize that again there's not a 1-to-1 correlation and that's probably the closest thing they resemble. The language bureaucrat in the back of my head complains, but I recognize that visually making processes distinct in color schemes has more practical benefits than some quibble with semantic naming that only a language purist would care about so I'm good with this too.
5) Again for what it's worth, null
is ACTUALLY a reserved word and not a constant. It's a statement that nothing happens (as opposed to words like error
, warning
, note
etc in the report command that is actually an enumerated type and thus operate like constants.) I'm okay with trying this one on for size but I may revisit this if it doesn't look right.
And I think that's about it. You said you had some things to clean up so let me know when you are satisfied. I wish I had some time to work down parts of my issues list but I'm neck deep in debugging another engineer's gigabit transceiver, and doing some research for a new project so I have been swamped for months. I REALLY appreciate the fact you are finding the package useful and sufficiently so to contribute to the project. (I'd really like to fix in-line comment aligning but it's not going to be as straightforward as just adding another align pass for comments because it'll completely screw up full line comments rolling into an in-line comment on the next line and I have to detect that whereas I don't believe I have a way of detecting it currently.) Anyhow, that's just my own ruminations. Thanks again for banging on this package and let me know when you are satisfied and I'll do the necessary documentation things (that won't take long.)
Oh one more thought, especially about the character literal comment. If you need to, you could pop open the test directory and look through that VHDL file. That is NOT a wholly valid VHDL file -- it won't compile. But it's my list of tests that I review before committing and I add tests that demonstrate some feature or function. I know I've got a ton of string literals in there you might check over to make sure your changes didn't break anything. String literals were one of the more complex portions of the syntax regular expressions because integer versus real versus base versus exponentials were all separate cases.
Just a thought. That's what that file is for.
Thanks for the feedback! In reverse order: I looked at the test file, and the string constants are still doing fine.
You are right about null
being a keyword: in my mind, I initially mapped it to the python None
and figured it should be styled similarly, but in reality it is much closer to pass
, which is a keyword. I have swapped it back.
I agree process
es are quite different from function
and procedure
. The effect on the highlighting is limited to the process's label name only at the moment. I could change this one back to the way it was perhaps?
About the cleanup: I've removed some commented lines. Also made a before / after screenshot: it seems like a move in the right direction to me, but I probably need to re-visit the abs
fix. I will post something here (and remove the [WIP] tag) when I've done that (probably tomorrow evening).
Also added some constructions in the screenshot that I do not have a good solution for yet (slicing getting recognized as a function call, lack of highlighting in default-assignments to local variables.
Also interesting: this fork is also improving the syntax file: https://github.com/emanuelen5/VHDL-Mode
I think process labels being labeled and identified with entity.name.function.process
is probably fine. I kind of remember way back when I started this that I did actually have those labels showing up highlighted because I was looking at python color-scheme and scopes and I was duplicating some things. Then later for some reason I switched to the current released solution. I think making them show up is probably a really good idea so I'm fine with that.
Very glad you caught some of those inline function name calls. Part of doing the syntax file early on was just a LOT of data entry. I was going through many appendices in the Ashenden book where he lists the prototypes for all the libraries and trying to catch all of those because they fall into support. And then there were the tables of defined math functions. In any event, those were banged in relatively quickly and clearly needed another pass for identifying word boundaries. Depending on how you name your signals though, you don't actually ever SEE that so it's just a problem waiting to happen.
As for correctly identifying a sliced vector as a function/procedure, well that is really hard due to the language. I knew that was in there. The problem is that any identifier name could be practically anything as a statement. If a solution exists, it probably lies in trying to do a lookahead for assignment. An identifier with or without a parentheical grouping operating on the left hand side of an assignment operator cannot be (to my knowledge) a function or procedure. A procedure will only ever be an identifier with or without a parenthetical grouping (parameters). Function is the hardest because it LOOKS like a vector identifier on the right hand side of the assignment. In fact it honestly could be! The following two text strings are different constructs but would ideally want them styled differently.
my_vector_name <= another_vector;
my_vector_name <= a_function_call_with_no_parameters;
Since the parenthetical parameter grouping is entirely optional, there's no regular expression way to detect that. So that's automatically going to be a failed scoping. However we can probably identify vectors versus procedures on the left hand side by doing a lookahead. The major problem there is that in the Sublime Text scoping state machine, you can't really tell what something isn't. You can only identify what something is. I think procedures are the LAST item on the list because it's simply an identifier followed by a semicolon. That is the most basic expression and it matches EVERYTHING. Maybe it could be made such that it checks for the lack of an assignment operator as lookahead as well.
As for the other forks, there've been a few here and there. I figure I made it pretty clear that I'm accepting of patches back into the main package so if they have something they want to kick in, it's fine with me. If or when Mr. Emanuel wants to send me a pull request I can evaluate it. Looking through it, it looks like he started adding syntax tests which is a great feature but for a language like VHDL it looked like a full time job coming up with the tests for that. I didn't have the time so I just went with my own subpar inspection testing.
Anyway, those are thoughts and notes I have from when I created this. Maybe helpful, probably not. :)
I've added the final 2 fixes I intended to include. Can be merged at your convenience if you think everything is ok.
I checked it over again and I think it looks good. I started writing up the messages file while reviewing as well. I'm going to accept the PR and then I'll check the issues list to see if there's anything that can be quickly knocked out and added (probably not -- there's a reason I haven't gotten to them) and implement what I can, and otherwise push a release. Thanks again for your hard work.
Wrong freakin' github button.
I've made some more modifications to the syntax definition file. Most changes relate to more precise scoping to allow detailed color schemes to use all their features, trying to bring things closer to https://www.sublimetext.com/docs/3/scope_naming.html.
Scoping both
function
andprocedure
names asentity.name.function.x
gives a better highlighting result I think (there is no real equivalent toprocedure
in most languages,function
is closest). I modified the tmPreference files to adapt the indexing to the new scoping. There might be other unforseen consequences I am not aware of, let me know if more changes are required here.One thing I'd consider a bugfix:
character-literal
rules were malformed, I've added the 'captures' section.Marked this as WIP for now, since I still need to clean up some commented code.