kernel: rename expression and statement "TNUMs" to avoid confusion with actual TNUMs #3422
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fingolfin
added
do not merge
fingolfin
requested review from
ChrisJefferson,
sebasguts,
stevelinton and
ssiccha
|
I would be totally happy with this PR, and also distinguishing stats and exprs is valid imho. I will try to fix #3371 soon (hopefully today or tomorrow) so we can get on with this. I assume you will leave the scripts in the PR, so I can apply them to other code I have already written using syntax trees? If not, it is also fine. |
stevelinton
reviewed
Apr 24, 2019
stevelinton
approved these changes
Apr 24, 2019
sebasguts
approved these changes
Apr 24, 2019
fingolfin
changed the title
fingolfin
changed the title
fingolfin
changed the title
Codecov Report
@@ Coverage Diff @@
## master #3422 +/- ##
==========================================
- Coverage 85.34% 85.34% -0.01%
==========================================
Files 699 699
Lines 346091 346092 +1
==========================================
- Hits 295373 295372 -1
- Misses 50718 50720 +2
|
fingolfin
commented
May 8, 2019
| @@ -330,7 +330,7 @@ static UInt ExecIfElifElse(Stat stat) | |||
| ** then 0 is returned. | |||
| ** | |||
| ** A for-loop with <n> statements in its body is represented by a bag of | |||
| ** type 'T_FOR' with <n>+2 subbags. The first subbag is an assignment bag | |||
| ** type 'STAT_FOR' with <n>+2 subbags. The first subbag is an assignment bag | |||
There was a problem hiding this comment.
There are still lots of these comments which claim that statements and expressions are "represented by a bag", and which talk about "subbags", and so on. I think it is out of scope for this PR, but it would be kinda nice to improve that.
E.g. instead of T_FUNCCALL_0ARGS we have EXPR_FUNCCALL_0ARGS, and instead of T_PROCCCALL_0ARGS we have STAT_PROCCCALL_0ARGS.
fingolfin
removed
the
do not merge
|
From my point of view, this could be merged now. |
|
I would be very happy if this can get merged. It conflicts with #3441 which I can update afterwards |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(This PR is motivated by repeated experiences trying to explain to people how GAP encodes function bodies. It is meant to make this code slightly more accessible.)
A long, long time ago, the statements and expressions which make up GAP function bodies were each coded as a separate GAP object. This is very inefficient for various reasons, so it was changed; now expressions and statements were simply a bunch of bytes within a single GAP object of type
T_BODY, with a specific format. Yet they all retained their "identity" in the form of a type number, such asT_WHILEto indicate a statement representing a while loop. To these day, the kernel code refers to these as "TNUMs".This can be rather confusing, as those TNUMs have nothing to do anymore with the GAP object TNUMs; yet they do not only still have the same name, but also they all share the same
T_prefix. E.g. we haveT_STRING(an object tnum) andT_STRING_EXPR(an expression tnum).We discussed this on slack some time ago. The consensus seemed to be that we should rename these, turning the
T_into eitherSTAT_orEXPR_, depending on the type.One alternative would be to use the same prefix for both, as there really is no difference between the two on a purely technical (low) level; the main importance for distinguishing the two cases is in validation of the coded function's structure, resp. whether in knowing which dispatch table to look when evaluating them. But for that it's not necessary to give them different names...
Alas, since a lot of code now tries hard to distinguish precisely between the two, I think it makes sense to go with the split here for now; if one day in the future we decided to unify expressions and statement terminology again, doing it for these "tnums" will be about the easiest part anyway ;-).
To perform the actual change, I wrote a little shell script, which I put into this PR. That makes it trivial to change things around: if we want another pair of prefixes: no problem. Also, if we merge some other PRs which will conflict with this PR (e.g. @sebasguts's PR #3371 which definitely should be merged before this PR), then it is trivial for me to recreate this PR.
Before this can be merged, some steps should be taken:
STAT_prefix (there are none withEXPR_prefix), namely:STAT_HEADERSTAT_MARK_CACHESTAT_LVARSSTAT_LVARS_PTR