Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Insert Dredd declartions before first function #220

Merged
merged 8 commits into from
Jul 17, 2024

Conversation

JamesLee-Jones
Copy link
Collaborator

Inserting Dredd's declarations at the beginning of the file can cause
issues with header files that have to be before any system headers. So
instead, insert Dredd's declarations at the top level before the first
mutation that we find.

Fixes: #219, #215

@JamesLee-Jones JamesLee-Jones linked an issue Apr 11, 2024 that may be closed by this pull request
@JamesLee-Jones
Copy link
Collaborator Author

Although this is unlikely to ever be unreasonably expensive, there is probably an optimizations that could be made. If the Clang AST is always traversed top down, then we just need to find the place that Dredd is going to modify and insert the prelude before this at the global scope. This means once start_location_of_first_function_in_source_file_ is valid, we could skip the rest of the function as this will always have the earliest starting source location.

@afd
Copy link
Member

afd commented Jul 11, 2024

@JamesLee-Jones Can you rebase this one please? From what I recall it should be good to go, except there was one simplification you were looking at making (see open discussion).

@JamesLee-Jones JamesLee-Jones force-pushed the change-dredd-declaration-location branch from 295b3b0 to bd59258 Compare July 11, 2024 10:36
@JamesLee-Jones JamesLee-Jones requested a review from afd July 11, 2024 11:11
Copy link
Member

@afd afd left a comment

Choose a reason for hiding this comment

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

One typo, and a suggestion for a small refactoring that I think will be worthwhile.

src/libdredd/src/mutate_visitor.cc Show resolved Hide resolved
src/libdredd/src/mutate_visitor.cc Outdated Show resolved Hide resolved
@afd
Copy link
Member

afd commented Jul 15, 2024

@JamesLee-Jones This will need a rebase. Does my suggested refactoring make sense and are you good to do it?

@JamesLee-Jones
Copy link
Collaborator Author

@JamesLee-Jones This will need a rebase. Does my suggested refactoring make sense and are you good to do it?

Your suggestion makes sense, I'll refactor it today!

@JamesLee-Jones JamesLee-Jones force-pushed the change-dredd-declaration-location branch from 6a8d335 to 4d0876d Compare July 15, 2024 22:16
Inserting Dredd's declarations at the beginning of the file can cause
issues with header files that have to be before any system headers. So
instead, insert Dredd's declarations at the top level before the first
mutation that we find.

Fixes: #219,#215
@JamesLee-Jones JamesLee-Jones force-pushed the change-dredd-declaration-location branch from 4a895bd to f0261de Compare July 16, 2024 08:09
@JamesLee-Jones JamesLee-Jones requested a review from afd July 16, 2024 09:02
Copy link
Member

@afd afd left a comment

Choose a reason for hiding this comment

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

Thanks! Good to go after making some very final changes.

@JamesLee-Jones JamesLee-Jones merged commit 24d06da into main Jul 17, 2024
9 checks passed
@JamesLee-Jones JamesLee-Jones deleted the change-dredd-declaration-location branch July 17, 2024 13:08
afd added a commit that referenced this pull request Aug 17, 2024
Related issues: #219, #215
Related PR: #220
@afd afd mentioned this pull request Aug 17, 2024
afd added a commit that referenced this pull request Aug 17, 2024
Related issues: #219, #215
Related PR: #220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants