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

misc: Fix the way --split-input-file is handled #2820

Closed
wants to merge 3 commits into from

Conversation

AntonLydike
Copy link
Collaborator

Make --split-input-file only split on // ----- if it's the first thing on a line (modulo whitespace)

@AntonLydike AntonLydike added the misc Miscellaneous label Jul 2, 2024
@AntonLydike AntonLydike self-assigned this Jul 2, 2024
@AntonLydike AntonLydike marked this pull request as ready for review July 2, 2024 16:31
Comment on lines +9 to +10
// CHECK: // -----

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can now CHECK for these markers!

Copy link
Collaborator

@PapyChacal PapyChacal left a comment

Choose a reason for hiding this comment

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

Yes, this annoyed me so many times 🤩

Why the modulo whitespace btw? Unless clear reason (or over complex implementation), I would also not split if preceding whitespace

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.91%. Comparing base (73c893c) to head (b72b25d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2820      +/-   ##
==========================================
+ Coverage   89.85%   89.91%   +0.05%     
==========================================
  Files         399      400       +1     
  Lines       50179    50313     +134     
  Branches     7755     7776      +21     
==========================================
+ Hits        45088    45238     +150     
+ Misses       3860     3847      -13     
+ Partials     1231     1228       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@math-fehr
Copy link
Collaborator

How do we handle this for non-.mlir files? For instance the frontend python files we still have?
I think it's common to write something like # // ----- for splitting these, why can we not CHECK: ----- for checking splits?

@AntonLydike
Copy link
Collaborator Author

Yes, this annoyed me so many times 🤩

Why the modulo whitespace btw? Unless clear reason (or over complex implementation), I would also not split if preceding whitespace

I found a bunch of tests that had indented splits, and I found that it's quite irritating to debug why the file wasn't split at a certain point if the split is indented by a single whitespace^^

Also, MLIR also allows indented splits, so yeah.

@AntonLydike
Copy link
Collaborator Author

How do we handle this for non-.mlir files? For instance the frontend python files we still have? I think it's common to write something like # // ----- for splitting these, why can we not CHECK: ----- for checking splits?

Seems weird that the tests pass... But good point!

Looking at MLIR, it just splits on any line containing // ----- anywhere, and deletes the whole line. Maybe we copy that?

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

fantastic

@math-fehr
Copy link
Collaborator

How do we handle this for non-.mlir files? For instance the frontend python files we still have? I think it's common to write something like # // ----- for splitting these, why can we not CHECK: ----- for checking splits?

Seems weird that the tests pass... But good point!

Looking at MLIR, it just splits on any line containing // ----- anywhere, and deletes the whole line. Maybe we copy that?

To me we should copy that yes, but what's currently implemented then? I thought it was already that

@AntonLydike
Copy link
Collaborator Author

How do we handle this for non-.mlir files? For instance the frontend python files we still have? I think it's common to write something like # // ----- for splitting these, why can we not CHECK: ----- for checking splits?

Seems weird that the tests pass... But good point!
Looking at MLIR, it just splits on any line containing // ----- anywhere, and deletes the whole line. Maybe we copy that?

To me we should copy that yes, but what's currently implemented then? I thought it was already that

We currently split on // ----- anywhere, but keep the remaining bits of the lines

I fixed it to eat the whole line.

@AntonLydike
Copy link
Collaborator Author

Wait, I was wrong. Current behaviour mimics MLIRs, so we are good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
misc Miscellaneous
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants