Skip to content

Conversation

@fruffy
Copy link
Collaborator

@fruffy fruffy commented Nov 7, 2025

No description provided.

…actually valid

Signed-off-by: fruffy <fruffy@nyu.edu>
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Nov 7, 2025
@ChrisDodd
Copy link
Contributor

Another possible approach would be to change YYLLOC_DEFAULT to something like:

#define YYLLOC_DEFAULT(Cur, Rhs, N)                                     \
    ((Cur) = (N) ? YYRHSLOC(Rhs, 1) + YYRHSLOC(Rhs, N) : yylloc)

This would set the location to the location of the lookahead token for an epsilon reduction.

Signed-off-by: fruffy <fruffy@nyu.edu>
@ChrisDodd
Copy link
Contributor

Perhaps better:

#define YYLLOC_DEFAULT(Cur, Rhs, N)                                             \
    ((Cur) = (N) ? YYRHSLOC(Rhs, 1) + YYRHSLOC(Rhs, N)                          \
        : yylloc ? yyloc \
                 : Util::SourceInfo(driver.sources, driver.sources.getSourcePosition()))

or

#define YYLLOC_DEFAULT(Cur, Rhs, N)                                             \
    ((Cur) = (N) ? YYRHSLOC(Rhs, 1) + YYRHSLOC(Rhs, N)                          \
        : YYRHSLOC(Rhs, 0) ?  Util::SourceInfo(driver.sources, YYRHSLOC(Rhs, 0).getEnd())  \
                 : Util::SourceInfo(driver.sources, driver.sources.getSourcePosition()))

Signed-off-by: fruffy <fruffy@nyu.edu>
@fruffy
Copy link
Collaborator Author

fruffy commented Nov 8, 2025

Perhaps better:

#define YYLLOC_DEFAULT(Cur, Rhs, N)                                             \
    ((Cur) = (N) ? YYRHSLOC(Rhs, 1) + YYRHSLOC(Rhs, N)                          \
        : yylloc ? yyloc \
                 : Util::SourceInfo(driver.sources, driver.sources.getSourcePosition()))

or

#define YYLLOC_DEFAULT(Cur, Rhs, N)                                             \
    ((Cur) = (N) ? YYRHSLOC(Rhs, 1) + YYRHSLOC(Rhs, N)                          \
        : YYRHSLOC(Rhs, 0) ?  Util::SourceInfo(driver.sources, YYRHSLOC(Rhs, 0).getEnd())  \
                 : Util::SourceInfo(driver.sources, driver.sources.getSourcePosition()))

It might actually be nicer to have the less complex version, I looked at the reference file failures, and they seemed to be benign. Mostly a difference of 1 character in terms of start/end.

@fruffy fruffy marked this pull request as ready for review November 8, 2025 17:47
@ChrisDodd
Copy link
Contributor

Simplest of all would be

#define YYLLOC_DEFAULT(Cur, Rhs, N)       \
    ((Cur) = (N) ? YYRHSLOC(Rhs, 1) + YYRHSLOC(Rhs, N) : YYRHSLOC(Rhs, 0))

which might be good enough. Would just sometimes make rules pull in an extra token if they involve an epsilon reduction.

or

#define YYLLOC_DEFAULT(Cur, Rhs, N)       \
    ((Cur) = (N) ? YYRHSLOC(Rhs, 1) + YYRHSLOC(Rhs, N) : Util::SrcInfo())

which would make all epsilons have an invalid srcInfo. But then the simple add would lose tokens for any rule with 3+ symbols when the first or last was an epsilon -- would be better to sum YYRHSLOC(Rhs, 1..N)

@ChrisDodd
Copy link
Contributor

Another possibility would be

#define YYLLOC_DEFAULT(Cur, Rhs, N) Cur = yylloc_default([](int i) { return YYRHSLOC(Rhs, i); }, N)

in all the .y/.ypp files and have a single

template <typename RHS> Util::SourceInfo yylloc_default(RHS rhs, int n) {
    Util::SourceInfo rv;
    for (int i = 1; i <= n; ++) rv += rhs(i);
    return rv;
}

in a header file to avoid the complex macro with line continuations

@fruffy fruffy force-pushed the fruffy/correct_parsing branch from 12959ed to bd938b1 Compare November 9, 2025 19:51
@fruffy
Copy link
Collaborator Author

fruffy commented Nov 9, 2025

Ended up reverting to the more elaborate version since there were small differences in the line count that led to failing tests.

@fruffy fruffy added this pull request to the merge queue Nov 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2025
@fruffy fruffy added this pull request to the merge queue Nov 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2025
@fruffy fruffy added this pull request to the merge queue Nov 23, 2025
Merged via the queue into main with commit 5da4bc9 Nov 23, 2025
38 checks passed
@fruffy fruffy deleted the fruffy/correct_parsing branch November 23, 2025 23:21
jhavrane pushed a commit to jhavrane/p4c that referenced this pull request Dec 19, 2025
…actually valid (p4lang#5417)

Signed-off-by: fruffy <fruffy@nyu.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Topics concerning the core segments of the compiler (frontend, midend, parser)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants