Skip to content

Conversation

@PietroGhg
Copy link
Contributor

The test used > to redirect stdout to a file, and then pipes (|) into cat, I'm not sure if it is intended but this PR removes the | so that cat works on the file created by the previous command.

@PietroGhg PietroGhg requested a review from a team as a code owner October 25, 2024 10:17
// verify that against the reference
// - ...and check if the list of improperly XFAIL-ed tests needs to be updated.
//
// RUN: grep -rI "XFAIL:" %S/../test-e2e \
Copy link
Contributor

@AndreiZibrov AndreiZibrov Oct 25, 2024

Choose a reason for hiding this comment

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

Correct.
The trailing redundant part ' | \$' proposed to be removed is just a blindly copied pasted ending with pipe from the previous lines. The line 31 has the ending part of the pipeline with first command started on line 28, so the | pipe with \ ending are redundant in the end of line 31 because next lines have no next part to be delimited with as done above.

Copy link
Contributor

@AndreiZibrov AndreiZibrov Oct 25, 2024

Choose a reason for hiding this comment

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

delimiter with pipeline " | \\(\s+)?" used around 60 times in llvm tests including SYCL

Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing \ should not be removed, because they are used to break multi-line commands. For example line 29 has no sense if it is launched standalone.
I'm also mostly sure that we can't remove \ in commands which use |, simply because of the way how lit handles those. Even though it looks like a shell script, it is strictly speaking isn't a shell script.

@AlexeySachkov
Copy link
Contributor

Tagging @sarnex for awareness, he also noticed that and fixes it in #15835

@PietroGhg
Copy link
Contributor Author

Tagging @sarnex for awareness, he also noticed that and fixes it in #15835

Thanks for pointing me to @sarnex 's PR. I guess we close this one if we expect #15835 to get merged soon

@sarnex
Copy link
Contributor

sarnex commented Oct 25, 2024

@PietroGhg I just merged #15835 so I think we can close this?

@PietroGhg
Copy link
Contributor Author

@PietroGhg I just merged #15835 so I think we can close this?

Agreed, thanks @sarnex

@PietroGhg PietroGhg closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants