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

chore: remove unused include from stl.h #3928

Merged
merged 2 commits into from
May 6, 2022
Merged

chore: remove unused include from stl.h #3928

merged 2 commits into from
May 6, 2022

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented May 6, 2022 •
edited by henryiii
Loading

Description

  • Removes the iostream include from stl.h. Only the header ostream is used which is included transitively.
  • Should help speed up compilation when pybind11/stl.h is included but istream objects are not needed.

Suggested changelog entry:

``#include <iostream>`` was removed from the ``pybind11/stl.h`` header. Your project may break if it has a transitive dependency on this include. The fix is to "Include What You Use".

@Skylion007 Skylion007 requested review from rwgk and henryiii May 6, 2022 15:57
@Skylion007 Skylion007 closed this May 6, 2022
@Skylion007 Skylion007 deleted the skylion007/remove-useless-iostream-include branch May 6, 2022 15:57
@Skylion007 Skylion007 restored the skylion007/remove-useless-iostream-include branch May 6, 2022 15:58
@Skylion007 Skylion007 reopened this May 6, 2022
@Skylion007 Skylion007 merged commit 2e33130 into pybind:master May 6, 2022
@Skylion007 Skylion007 deleted the skylion007/remove-useless-iostream-include branch May 6, 2022 20:57
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 6, 2022
@rwgk
Copy link
Collaborator

rwgk commented May 18, 2022 •
edited
Loading

For the record / entertainment: This turns out to be a breaking change: Google-global testing hit 5 projects (including https://github.com/google/atheris) that are broken by this PR (14 broken tests in total).

We should mention this in the changelog. The global testing suggests a number of people will stumble over this.

@Skylion007
Copy link
Collaborator Author

@rwgk I updated the changelog to be more specific (we still include iostream.h in pybind11/iostream.h).

wangxf123456 pushed a commit to google/clif that referenced this pull request Jun 4, 2022
@wangxf123456
Import pybind/pybind11 from GitHub (pybind/pybind11#3957).
39db7ae 
* Includes pybind/pybind11#3844 — [smart_holder] .def_readonly, .def_readwrite adaptors

* Also removing no-longer-needed workaround from third_party/clif/testing/python/nested_fields_test.py

* third_party/pybind11/google3_patches/stl_h_include_iostream.patch undoes pybind/pybind11#3928, to unblock this CL. The patch will be removed after missing `#include <iostream>` are added to ~5 projects that currently have a transitive dependency through pybind11 (for broken targets fixed by this patch see https://fusion2.corp.google.com/presubmit/tap/449354800/OCL:449354800:BASE:449423292:1652863858736:bd18ffcd;groups=Passing/targets).

  - 2e331308d38a521c087e7fc0cfee227cd29f3f71 chore: remove unused include from stl.h (#3928) by Aaron Gokaslan <skylion.aaron@gmail.com>
  - ad146b2a1877e8ba3803f94a7837969835a297a7 [pre-commit.ci] pre-commit autoupdate (#3933) by pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
  - 5621ab853a60ad48bce08487cc6e220930178b79 Do we have a unit test for the traceback code in error_st... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 48c7be4a5643cdf48a1228de05f6279ec95e99d3 Undoing previous accidental commit. Sorry I forgot to git... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 72eea20afd51e363fe115265043c2b2b6bcc523a Fix py::cast from pytype rvalue to pytype (#3949) by Maarten Baert <maarten-baert@hotmail.com>
  - 1a7b12983e09f698be3007b5868bfdf931d9a4d1 ci: fix cuda issue & MSVC spurious warning (#3950) by Henry Schreiner <HenrySchreinerIII@gmail.com>
  - dff6fa0554bf6efe98a8da3f932b749cff4d76a8 fix(cmake): avoid issue with NVCC + Windows (#3947) by Henry Schreiner <HenrySchreinerIII@gmail.com>
  - a8b3ff30f9649459021adc80f98a945d3ac675a5 chore: add a couple of moves in pybind11.h (#3941) by Aaron Gokaslan <skylion.aaron@gmail.com>
  - d28c3a5da7a199530be017000ba4dfb2ff812624 [smart_holder] .def_readonly, .def_readwrite adaptors (co... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 50220aeb09ac6ae02b6dd95fdc78f7c537920068 Tracking ci.yml changes from master. by Ralf W. Grosse-Kunstleve <rwgk@google.com>

PiperOrigin-RevId: 449521012
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 7, 2022
AidenRHall pushed a commit to google/atheris that referenced this pull request Jul 25, 2022
#include <iostream> fixes.
4bf0138 
[pybind11 PR #3928](pybind/pybind11#3928) removed `#include <iostream>` from pybind11/stl.h.

For compatibility with pybind11 v2.10+, `#include <iostream>` needs to be added to sources that currently depend on this include transitively through pybind11/stl.h.

PiperOrigin-RevId: 463082349
Change-Id: I96c927860ad32d023b7b8d2c66a8364da3059fb1
@rwgk rwgk mentioned this pull request Feb 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwgk rwgk rwgk approved these changes

@henryiii henryiii Awaiting requested review from henryiii

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Skylion007 @rwgk @henryiii