Refactor AST to Enum-based & Implement OpenACC 3.4 'indirect' Clause#15
Refactor AST to Enum-based & Implement OpenACC 3.4 'indirect' Clause#15
Conversation
Summary of ChangesHello @ouankou, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Abstract Syntax Tree (AST) to improve its internal representation of OpenACC clauses. By transitioning from string-based keyword identification to a more structured enum-based approach, the system gains better type safety and reduces the potential for errors. Concurrently, it introduces full compliance for the OpenACC 3.4 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant refactoring and new feature support for OpenACC clause parsing and representation. Key changes include adding support for the indirect clause, enhancing the parsing of gang clause arguments, and improving the handling of device_type items. The parsing logic for bind, collapse, default, and reduction clauses has been updated to use direct context checks instead of string comparisons, leading to more robust and efficient parsing. A new OpenACCDataClause base class and addModifier method were introduced to consolidate the handling of data clause modifiers (e.g., always, readonly, zero) across copy, copyin, copyout, and create clauses, replacing older, less flexible modifier mechanisms. Correspondingly, the OpenACCIR.h file was updated to reflect these new class hierarchies and removed redundant modifier-specific functions. The OpenACCIRToString.cpp file was adjusted to correctly print the new indirect clause, collapse clause with force modifier, and gang arguments, and to use * for any device type. The lexer and parser grammars (acclexer.g4, accparser.g4) were extensively modified to support these new syntax elements and parsing strategies, including new lexer modes for bind, collapse, copy, copyin, copyout, create, and device_type clauses, and new tokens for data clause modifiers. A new test file indirect.txt and its reference output were added. Review comments highlighted the need to strip quotes from string literals in the indirect clause for correct output, suggested using static_cast for type safety, and recommended simplifying repeated code for extracting gang argument values. A memory leak fix was noted for deleting merged clauses.
| std::string text = | ||
| trimEnclosingWhiteSpace(ctx->name_or_string()->getText()); | ||
| bool is_str = ctx->name_or_string()->STRING_LITERAL() != nullptr; | ||
| indirect->setValue(text, is_str); |
There was a problem hiding this comment.
When a string literal is parsed, ctx->name_or_string()->getText() includes the enclosing quotes. Storing the value with quotes and then adding another pair of quotes in OpenACCIndirectClause::toString() will result in incorrect output (e.g., ""foo""). The enclosing quotes should be stripped here before setting the value. This will ensure the toString() method can correctly reconstruct the pragma.
std::string text =
trimEnclosingWhiteSpace(ctx->name_or_string()->getText());
bool is_str = ctx->name_or_string()->STRING_LITERAL() != nullptr;
if (is_str && text.length() >= 2) {
if ((text.front() == '"' && text.back() == '"') || (text.front() == '\'' && text.back() == '\'')) {
text = text.substr(1, text.length() - 2);
}
}
indirect->setValue(text, is_str);| if (ctx->NUM()) { | ||
| kind = ACCC_GANG_ARG_num; | ||
| if (ctx->int_expr()) { | ||
| value_text = trimEnclosingWhiteSpace(ctx->int_expr()->getText()); | ||
| } | ||
| } else if (ctx->DIM()) { | ||
| kind = ACCC_GANG_ARG_dim; | ||
| if (ctx->int_expr()) { | ||
| value_text = trimEnclosingWhiteSpace(ctx->int_expr()->getText()); | ||
| } | ||
| } else if (ctx->STATIC()) { | ||
| kind = ACCC_GANG_ARG_static; | ||
| if (ctx->int_expr()) { | ||
| value_text = trimEnclosingWhiteSpace(ctx->int_expr()->getText()); | ||
| } | ||
| } else if (ctx->int_expr()) { | ||
| kind = ACCC_GANG_ARG_num_no_keyword; | ||
| value_text = trimEnclosingWhiteSpace(ctx->int_expr()->getText()); | ||
| } |
There was a problem hiding this comment.
The logic to extract value_text from ctx->int_expr() is repeated multiple times within the if-else if chain. This can be simplified by extracting the expression text once at the beginning, which improves readability and reduces code duplication.
if (ctx->int_expr()) {
value_text = trimEnclosingWhiteSpace(ctx->int_expr()->getText());
}
if (ctx->NUM()) {
kind = ACCC_GANG_ARG_num;
} else if (ctx->DIM()) {
kind = ACCC_GANG_ARG_dim;
} else if (ctx->STATIC()) {
kind = ACCC_GANG_ARG_static;
} else if (ctx->int_expr()) {
kind = ACCC_GANG_ARG_num_no_keyword;
}| mergeVarList(existing, incoming); | ||
| current_clauses->pop_back(); | ||
| directive->getClausesInOriginalOrder()->pop_back(); | ||
| delete incoming; |
There was a problem hiding this comment.
It's great that you're deleting the incoming clause object after merging to prevent a memory leak. This is a critical fix for correctness.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::string text = | ||
| trimEnclosingWhiteSpace(ctx->name_or_string()->getText()); | ||
| bool is_str = ctx->name_or_string()->STRING_LITERAL() != nullptr; | ||
| if (is_str && text.length() >= 2) { |
There was a problem hiding this comment.
The new indirect clause relies on STRING_LITERAL to decide whether to strip quotes (is_str = ctx->name_or_string()->STRING_LITERAL() != nullptr), but the lexer only defines STRING_LITERAL in bind_clause mode (see acclexer.g4 around mode bind_clause, while INDIRECT pushes expr_clause at acclexer.g4 lines ~361–366). As a result, #pragma acc routine indirect("foo") is tokenized as EXPR, is_str stays false, and the AST keeps the quoted text as a non-string value. This changes clause semantics for valid string-literal forms. Consider adding STRING_LITERAL to expr_clause/expression_mode or falling back to quote-detection for EXPR in this path.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| data_clause_in_modifier_list = | ||
| lookAheadToken("always") || lookAheadToken("alwaysin") || lookAheadToken("alwaysout") || | ||
| lookAheadToken("capture") || lookAheadToken("readonly") || lookAheadToken("zero"); | ||
| if (!data_clause_in_modifier_list) { |
There was a problem hiding this comment.
The modifier detection now uses lookAheadToken(...) to decide whether the text after copy(/copyin(/etc. is a modifier list. That means identifiers that exactly match modifier keywords (e.g., always, alwaysin, alwaysout, capture, readonly, zero) are no longer parseable as variable names because the lexer assumes a modifier list and the parser then requires a colon. For example, #pragma acc data copy(always) (a valid variable name in C/Fortran) will now be rejected as a syntax error. This is a regression for any code that uses identifiers equal to those keywords.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
This PR refactors the AST to remove string-based hacks in favor of enum-based logic and implements full support for the OpenACC 3.4 'indirect' clause.
Key Changes
original_keywordin data clauses withOpenACCDataClauseVariantenums.indirectclause forroutinedirective, supporting bothindirectandindirect(name).exitName_or_stringby correcting unsafe casting logic inOpenACCASTConstructor.acclexer.g4andaccparser.g4to support the new token and rules.tests/builtin/indirect.txt.Verification
ctest).