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

Include missing headers #517

Merged
merged 3 commits into from
Oct 3, 2023
Merged

Include missing headers #517

merged 3 commits into from
Oct 3, 2023

Conversation

MichaelChirico
Copy link
Contributor

@MichaelChirico MichaelChirico commented Oct 2, 2023

  • cpp11.cpp: <cstddef> for ptrdiff_t
  • vroom_write.cc: for <functional> std::cref

Our tooling also recommends directly including the following cpp11 headers to src/cpp11.cpp, but I omitted that as it feels more stylistic:

  • "cpp11/as.hpp" for as_cpp and as_sexp
  • "cpp11/integers.hpp" for integers

Happy to update the patch to include those if desired.

src/cpp11.cpp Outdated
@@ -4,6 +4,7 @@
#include "vroom_types.h"
#include "cpp11/declarations.hpp"
#include <R_ext/Visibility.h>
#include <cstddef>
Copy link
Member

Choose a reason for hiding this comment

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

This is a generated file, so if this is important, we'll have to make different arrangements.

Copy link
Member

@DavisVaughan DavisVaughan Oct 3, 2023

Choose a reason for hiding this comment

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

I think if we were going to put this anywhere, it would be in vroom_types.h, since the idea of that file is that it gets automatically included here to provide the definitions for any types needed to support this autogenerated file, like vroom_errors

cpp11::data_frame vroom_errors_(cpp11::external_pointer<std::shared_ptr<vroom_errors>> errors);

But I'm not super worried about this one because cpp11/declarations.hpp includes cpp11/cpp11.hpp which includes all of cpp11, and cpp11/r_vector.hpp includes <stddef.h> which is where ptrdiff_t is (https://cplusplus.com/reference/cstddef/) and I don't really see that changing. Typically I am more worried about implicit dependencies like this, but this file is kind of an exception.


AFAICT, it was very intentional that cpp11/declarations.hpp includes cpp11/cpp11.hpp. It basically includes that file + adds a few other declarations needed for code.cpp. But the idea is that including cpp11/declarations.hpp is "enough" to generate code.cpp without any other cpp11 headers. So we don't need to add cpp11/as.hpp or cpp11/integers.hpp either
https://github.com/r-lib/cpp11/blob/500f642b4ea132ec8c168fc70a28e81e9510ece3/inst/include/cpp11/declarations.hpp#L7-L16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if CRAN starts enforcing on this clang finding, it's gonna be chaos for sure.

Copy link
Member

Choose a reason for hiding this comment

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

So we're agreed, I think, that this change should be reverted.

Then I'll merge the PR with the remaining change.

@@ -1,5 +1,6 @@
#include "grisu3.h"
#include <array>
#include <functional>
Copy link
Member

Choose a reason for hiding this comment

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

Seems fine to me. Do you have an opinion @DavisVaughan?

Copy link
Member

Choose a reason for hiding this comment

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

Looks right to me since cref() does indeed come from <functional>
https://en.cppreference.com/w/cpp/utility/functional/ref

@jennybc
Copy link
Member

jennybc commented Oct 3, 2023

Thanks!

@jennybc jennybc merged commit 7eef177 into tidyverse:main Oct 3, 2023
13 checks passed
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