Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

ast: add Location to wrap parser location #3867

Merged
merged 1 commit into from
Mar 6, 2025
Merged

ast: add Location to wrap parser location #3867

merged 1 commit into from
Mar 6, 2025

Conversation

amscanne
Copy link
Contributor

@amscanne amscanne commented Mar 3, 2025 •
edited
Loading

Stacked PRs:

ast: add Location to wrap parser location

In the future, we will no longer rely on the global singleton to contain
the filename and contents, since we want to support multiple files. This
change introduces an ASTSource class which can be used to contain this
metadata, and wraps a reference to this in a new Location class.

For now, the existing filename and contents are used by the LogSink,
but this will change very shortly.

One related note with this change is the requirement that Location be
passed to nodes as a r-value. This is a minor change, but means that
calls to make_node internally must explicitly copy or construct a new
location, which makes it clear what is happening. In general, the
Location type is cheap and can be carried by value.

Signed-off-by: Adin Scannell amscanne@meta.com

@amscanne amscanne force-pushed the user/amscanne/diags/9 branch from a7edc08 to db47a19 Compare March 3, 2025 21:34
This was referenced Mar 3, 2025
Merged
Open
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 3, 2025
View reviewed changes
src/ast/ast.cpp
{
}

Sizeof::Sizeof(Diagnostics &d, SizedType type, location loc)
: Expression(d, loc), argtype(type)
Sizeof::Sizeof(Diagnostics &d, SizedType type, Location &&loc)

Check notice

Code scanning / CodeQL

Large object passed by value Note

This parameter of type
SizedType
Loading
is 160 bytes - consider passing a const pointer/reference instead.

Copilot Autofix AI 9 days ago

To fix the problem, we should pass the SizedType object by reference instead of by value. This involves changing the constructor of the Sizeof class to accept a const SizedType& instead of a SizedType. This change will ensure that only a reference to the object is passed, avoiding the overhead of copying the large object.

  • Change the constructor signature in the Sizeof class to accept a const SizedType& for the type parameter.
  • Update the constructor definition accordingly.
Suggested changeset 1
src/ast/ast.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/ast/ast.cpp b/src/ast/ast.cpp
--- a/src/ast/ast.cpp
+++ b/src/ast/ast.cpp
@@ -64,3 +64,3 @@
 
-Sizeof::Sizeof(Diagnostics &d, SizedType type, Location &&loc)
+Sizeof::Sizeof(Diagnostics &d, const SizedType &type, Location &&loc)
     : Expression(d, std::move(loc)), argtype(type)
EOF
@@ -64,3 +64,3 @@

Sizeof::Sizeof(Diagnostics &d, SizedType type, Location &&loc)
Sizeof::Sizeof(Diagnostics &d, const SizedType &type, Location &&loc)
: Expression(d, std::move(loc)), argtype(type)
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
src/ast/ast.cpp
Cast::Cast(Diagnostics &d, SizedType cast_type, Expression *expr, location loc)
: Expression(d, loc), expr(expr)
Cast::Cast(Diagnostics &d,
SizedType cast_type,

Check notice

Code scanning / CodeQL

Large object passed by value Note

This parameter of type
SizedType
Loading
is 160 bytes - consider passing a const pointer/reference instead.

Copilot Autofix AI 9 days ago

To fix the problem, we should pass the SizedType object by reference instead of by value. This will avoid the overhead of copying the object and reduce the risk of stack overflow. Specifically, we will change the parameter type from SizedType to const SizedType& in the Cast constructor.

  • Change the parameter type of cast_type in the Cast constructor from SizedType to const SizedType&.
  • Update the constructor initialization list to use the reference.
Suggested changeset 1
src/ast/ast.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/ast/ast.cpp b/src/ast/ast.cpp
--- a/src/ast/ast.cpp
+++ b/src/ast/ast.cpp
@@ -164,8 +164,7 @@
 Cast::Cast(Diagnostics &d,
-           SizedType cast_type,
+           const SizedType &cast_type,
            Expression *expr,
            Location &&loc)
-    : Expression(d, std::move(loc)), expr(expr)
+    : Expression(d, std::move(loc)), expr(expr), type(cast_type)
 {
-  type = cast_type;
 }
EOF
@@ -164,8 +164,7 @@
Cast::Cast(Diagnostics &d,
SizedType cast_type,
const SizedType &cast_type,
Expression *expr,
Location &&loc)
: Expression(d, std::move(loc)), expr(expr)
: Expression(d, std::move(loc)), expr(expr), type(cast_type)
{
type = cast_type;
}
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@amscanne
ast: add Location to wrap parser location
90fc2f7 
In the future, we will no longer rely on the global singleton to contain
the filename and contents, since we want to support multiple files. This
change introduces an `ASTSource` class which can be used to contain this
metadata, and wraps a reference to this in a new `Location` class.

For now, the existing filename and contents are used by the `LogSink`,
but this will change very shortly.

One related note with this change is the requirement that `Location` be
passed to nodes as a r-value. This is a minor change, but means that
calls to `make_node` internally must explicitly copy or construct a new
location, which makes it clear what is happening. In general, the
`Location` type is cheap and can be carried by value.

Signed-off-by: Adin Scannell <amscanne@meta.com>

stack-info: PR: #3867, branch: user/amscanne/diags/9
@amscanne amscanne force-pushed the user/amscanne/diags/9 branch from db47a19 to 90fc2f7 Compare March 4, 2025 00:54
@amscanne amscanne mentioned this pull request Mar 4, 2025
Open
@amscanne amscanne marked this pull request as ready for review March 4, 2025 03:06
jordalgo
jordalgo reviewed Mar 4, 2025
View reviewed changes
src/ast/context.h
// this automatically converts the parser `location` to the `Location` class
// bound to the current source file.
template <typename T>
auto wrap(T &&t) -> decltype(t)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JC, what else do we intend on wrapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing at the moment, but this is also the simplest way to automatically decay all the regular cases to the same type.

jordalgo
jordalgo approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@jordalgo jordalgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly reviewed the new classes and skimmed the rest (relying on our tests here).

@danobi danobi merged commit 7d9ad93 into master Mar 6, 2025
19 checks passed
@danobi danobi deleted the user/amscanne/diags/9 branch March 6, 2025 18:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@danobi danobi danobi approved these changes

@jordalgo jordalgo jordalgo approved these changes

@ajor ajor Awaiting requested review from ajor ajor is a code owner

@viktormalik viktormalik Awaiting requested review from viktormalik viktormalik is a code owner

@fbs fbs Awaiting requested review from fbs fbs is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amscanne @danobi @jordalgo