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.

Sign up for GitHub

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

Jump to bottom

#include <iostream> makes it impossible to use pipe as a variable name #79782

Closed
hewillk opened this issue Jan 29, 2024 · 4 comments · Fixed by #80464
Closed

#include <iostream> makes it impossible to use pipe as a variable name #79782

hewillk opened this issue Jan 29, 2024 · 4 comments · Fixed by #80464
Assignees
@mordante
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@hewillk
Copy link

hewillk commented Jan 29, 2024 

#include <iostream>

int pipe;

https://godbolt.org/z/fqPhTzq1E
The latest libc++ rejects the above code with:

<source>:3:5: error: redefinition of 'pipe' as different kind of symbol
    3 | int pipe;
      |     ^
/usr/include/unistd.h:437:12: note: previous definition is here
  437 | extern int pipe (int __pipedes[2]) __THROW __wur;
      |            ^
1 error generated.

Note that Clang-17's libc++, libc++, and MSVC-STL accept it.
@EugeneZelenko EugeneZelenko added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. and removed new issue labels Jan 29, 2024
@mordante
Copy link
Member

Mainly curious where did you spot this?
The issue is due to including unistd.h which is needed for isatty. I'm surprised it's not an issue for libstdc++, I'm sure they use isatty too (https://cplusplus.github.io/LWG/issue4044).

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Jan 30, 2024 •
edited

Mainly curious where did you spot this? The issue is due to including unistd.h which is needed for isatty. I'm surprised it's not an issue for libstdc++, I'm sure they use isatty too (https://cplusplus.github.io/LWG/issue4044).

libstdc++ only includes <unistd.h> in a separately compiled source file in order to avoid name conflict.
https://github.com/gcc-mirror/gcc/blob/f22a7ae8a96f7e5e330b12bd5045424619aa4926/libstdc%2B%2B-v3/src/c%2B%2B23/print.cc

@hewillk
Copy link
Author

hewillk commented Jan 30, 2024 •
edited

Mainly curious where did you spot this?

I had a small piece of code that defined a namespace named pipe, and then suddenly found that it could not be compiled in clang-trunk.

@mordante mordante mentioned this issue Feb 2, 2024
Merged
mordante added a commit to mordante/llvm-project that referenced this issue Feb 2, 2024
@mordante
[libc++][print] Moves is_terminal to the dylib.
4e6789a 
Having the test in the header requires including unistd.h on POSIX
platforms. This header has other declarations which may conflict with
code that uses named declarations provided by this header. For example
code using "int pipe;" would conflict with the function pipe in this
header.

Moving the code to the dylib means std::print would not be available on
Apple backdeployment targets. On POSIX platforms there is no transcoding
required so a not Standard conforming implementation is still a useful
and the observable differences are minimal. This behaviour has been done
for print before llvm#76293.

Note questions have been raised in LWG4044 "Confusing requirements for
std::print on POSIX platforms", whether or not the isatty check on POSIX
platforms is required. When this LWG issue is resolved the
backdeployment targets could become Standard compliant.

This patch is intended to be backported to the LLVM-18 branch.

Fixes: llvm#79782
@mordante
Copy link
Member

mordante commented Feb 2, 2024

Thanks for the info. I'm testing a patch now once approved I'll request to cherry-pick it to the LLVM-18 release branch.

mordante added a commit to mordante/llvm-project that referenced this issue Feb 3, 2024
@mordante
[libc++][print] Moves is_terminal to the dylib.
4a6e148 
Having the test in the header requires including unistd.h on POSIX
platforms. This header has other declarations which may conflict with
code that uses named declarations provided by this header. For example
code using "int pipe;" would conflict with the function pipe in this
header.

Moving the code to the dylib means std::print would not be available on
Apple backdeployment targets. On POSIX platforms there is no transcoding
required so a not Standard conforming implementation is still a useful
and the observable differences are minimal. This behaviour has been done
for print before llvm#76293.

Note questions have been raised in LWG4044 "Confusing requirements for
std::print on POSIX platforms", whether or not the isatty check on POSIX
platforms is required. When this LWG issue is resolved the
backdeployment targets could become Standard compliant.

This patch is intended to be backported to the LLVM-18 branch.

Fixes: llvm#79782
mordante added a commit to mordante/llvm-project that referenced this issue Feb 9, 2024
@mordante
[libc++][print] Moves is_terminal to the dylib.
20d0e48 
Having the test in the header requires including unistd.h on POSIX
platforms. This header has other declarations which may conflict with
code that uses named declarations provided by this header. For example
code using "int pipe;" would conflict with the function pipe in this
header.

Moving the code to the dylib means std::print would not be available on
Apple backdeployment targets. On POSIX platforms there is no transcoding
required so a not Standard conforming implementation is still a useful
and the observable differences are minimal. This behaviour has been done
for print before llvm#76293.

Note questions have been raised in LWG4044 "Confusing requirements for
std::print on POSIX platforms", whether or not the isatty check on POSIX
platforms is required. When this LWG issue is resolved the
backdeployment targets could become Standard compliant.

This patch is intended to be backported to the LLVM-18 branch.

Fixes: llvm#79782
mordante added a commit to mordante/llvm-project that referenced this issue Feb 10, 2024
@mordante
[libc++][print] Moves is_terminal to the dylib.
28e9c94 
Having the test in the header requires including unistd.h on POSIX
platforms. This header has other declarations which may conflict with
code that uses named declarations provided by this header. For example
code using "int pipe;" would conflict with the function pipe in this
header.

Moving the code to the dylib means std::print would not be available on
Apple backdeployment targets. On POSIX platforms there is no transcoding
required so a not Standard conforming implementation is still a useful
and the observable differences are minimal. This behaviour has been done
for print before llvm#76293.

Note questions have been raised in LWG4044 "Confusing requirements for
std::print on POSIX platforms", whether or not the isatty check on POSIX
platforms is required. When this LWG issue is resolved the
backdeployment targets could become Standard compliant.

This patch is intended to be backported to the LLVM-18 branch.

Fixes: llvm#79782
mordante added a commit that referenced this issue Feb 10, 2024
@mordante
[libc++][print] Moves is_terminal to the dylib. (#80464)
4fb7b33 
Having the test in the header requires including unistd.h on POSIX
platforms. This header has other declarations which may conflict with
code that uses named declarations provided by this header. For example
code using "int pipe;" would conflict with the function pipe in this
header.

Moving the code to the dylib means std::print would not be available on
Apple backdeployment targets. On POSIX platforms there is no transcoding
required so a not Standard conforming implementation is still a useful
and the observable differences are minimal. This behaviour has been done
for print before #76293.

Note questions have been raised in LWG4044 "Confusing requirements for
std::print on POSIX platforms", whether or not the isatty check on POSIX
platforms is required. When this LWG issue is resolved the
backdeployment targets could become Standard compliant.

This patch is intended to be backported to the LLVM-18 branch.

Fixes: #79782
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 11, 2024
@mordante @llvmbot
[libc++][print] Moves is_terminal to the dylib. (llvm#80464)
460dc4d 
Having the test in the header requires including unistd.h on POSIX
platforms. This header has other declarations which may conflict with
code that uses named declarations provided by this header. For example
code using "int pipe;" would conflict with the function pipe in this
header.

Moving the code to the dylib means std::print would not be available on
Apple backdeployment targets. On POSIX platforms there is no transcoding
required so a not Standard conforming implementation is still a useful
and the observable differences are minimal. This behaviour has been done
for print before llvm#76293.

Note questions have been raised in LWG4044 "Confusing requirements for
std::print on POSIX platforms", whether or not the isatty check on POSIX
platforms is required. When this LWG issue is resolved the
backdeployment targets could become Standard compliant.

This patch is intended to be backported to the LLVM-18 branch.

Fixes: llvm#79782
(cherry picked from commit 4fb7b33)
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 16, 2024
@mordante @tstellar
[libc++][print] Moves is_terminal to the dylib. (llvm#80464)
872fe20 
Having the test in the header requires including unistd.h on POSIX
platforms. This header has other declarations which may conflict with
code that uses named declarations provided by this header. For example
code using "int pipe;" would conflict with the function pipe in this
header.

Moving the code to the dylib means std::print would not be available on
Apple backdeployment targets. On POSIX platforms there is no transcoding
required so a not Standard conforming implementation is still a useful
and the observable differences are minimal. This behaviour has been done
for print before llvm#76293.

Note questions have been raised in LWG4044 "Confusing requirements for
std::print on POSIX platforms", whether or not the isatty check on POSIX
platforms is required. When this LWG issue is resolved the
backdeployment targets could become Standard compliant.

This patch is intended to be backported to the LLVM-18 branch.

Fixes: llvm#79782
(cherry picked from commit 4fb7b33)
blueboxd pushed a commit to blueboxd/libcxx that referenced this issue May 4, 2024
@mordante
[libc++][print] Moves is_terminal to the dylib. (#80464)
dac1af6 
Having the test in the header requires including unistd.h on POSIX
platforms. This header has other declarations which may conflict with
code that uses named declarations provided by this header. For example
code using "int pipe;" would conflict with the function pipe in this
header.

Moving the code to the dylib means std::print would not be available on
Apple backdeployment targets. On POSIX platforms there is no transcoding
required so a not Standard conforming implementation is still a useful
and the observable differences are minimal. This behaviour has been done
for print before llvm/llvm-project#76293.

Note questions have been raised in LWG4044 "Confusing requirements for
std::print on POSIX platforms", whether or not the isatty check on POSIX
platforms is required. When this LWG issue is resolved the
backdeployment targets could become Standard compliant.

This patch is intended to be backported to the LLVM-18 branch.

Fixes: llvm/llvm-project#79782
NOKEYCHECK=True
GitOrigin-RevId: 4fb7b3301bfbd439eb3d30d6a36c7cdb26941a0d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mordante mordante

Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[libc++][print] Moves is_terminal to the dylib. mordante/llvm-project
4 participants
@mordante @EugeneZelenko @frederick-vs-ja @hewillk