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

Prevent the preprocessor from replacing std{in,out} with macro #2111

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

vmcj
Copy link
Member

@vmcj vmcj commented Jul 29, 2023

Trying to build on alpine (which uses musl instead of glibc) would break as the preprocessor replaces stdin->(stdin) which fails to compile.

After discussion with @ankon he found:
https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130506/173524.html

@vmcj vmcj requested a review from edomora97 July 29, 2023 10:13
@vmcj
Copy link
Member Author

vmcj commented Jul 29, 2023

build fails, but the basic question is if we want to keep the naming (and have to fix the macro) or if there is a proper name which is not taken by a macro which can work.

judge/runpipe.cc Outdated
@@ -641,22 +641,22 @@ struct state_t {
tie(read_end, write_end) = make_pipe();
logmsg(LOG_DEBUG, "setting up pipe #%ld (fd %d) -> proxy (fd %d)", i,
write_end, read_end);
process.stdout = write_end;
process.pstdout = write_end;
Copy link
Member

Choose a reason for hiding this comment

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

What does p stand for? I think this needs a clearer variable name. I don't know the context well enough to choose the best one, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. There is the need to rename this variable, but I don't like pstdin/pstdout either. Here some suggestions (in my decreasing order of preference):

  • standard_input
  • stdin_fd
  • input
  • stdin_

Copy link
Member Author

@vmcj vmcj Jul 31, 2023

Choose a reason for hiding this comment

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

P was for process as placeholder,

I'm fine with either standard_{in,out}put or std{in,out}_fd, @eldering do you have a preference? In the end it should be clear for the person doing the debugging which will probably not be me.

If no response by wednesday I'll pick stdin_fd and merge (as its the shortest and its trivial to rename later and I need this for my next PR).

Copy link
Member

Choose a reason for hiding this comment

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

std{in,out}_fd sounds fine to me.

Copy link
Contributor

@edomora97 edomora97 left a comment

Choose a reason for hiding this comment

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

Thanks! Overall looks good, but I agree with Jaap that we need a better name for this 😬

judge/runpipe.cc Outdated
@@ -641,22 +641,22 @@ struct state_t {
tie(read_end, write_end) = make_pipe();
logmsg(LOG_DEBUG, "setting up pipe #%ld (fd %d) -> proxy (fd %d)", i,
write_end, read_end);
process.stdout = write_end;
process.pstdout = write_end;
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. There is the need to rename this variable, but I don't like pstdin/pstdout either. Here some suggestions (in my decreasing order of preference):

  • standard_input
  • stdin_fd
  • input
  • stdin_

Trying to build on alpine (which uses musl instead of glibc) would break
as the preprocessor replaces stdin->(stdin) which fails to compile.

After discussion with @ankon he found:
https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130506/173524.html
@vmcj vmcj merged commit d0007b5 into DOMjudge:main Aug 1, 2023
17 checks passed
@vmcj vmcj deleted the build_musl_runpipe branch August 1, 2023 16:28
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.

3 participants