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

Add missing includes #1272

Merged
merged 4 commits into from
Aug 3, 2023
Merged

Add missing includes #1272

merged 4 commits into from
Aug 3, 2023

Conversation

MichaelChirico
Copy link
Contributor

@MichaelChirico MichaelChirico commented Aug 3, 2023

Minor cleanup, no ticket.

Feel free to suggest another ordering of #includes, picked somewhat haphazardly

@eddelbuettel
Copy link
Member

eddelbuettel commented Aug 3, 2023

What OS / platform / compiler triggers what error here?

As you know we usually hear from CRAN when something is bad, or is missing, or ... and Prof Ripley is pretty good at getting to new and not-always-relased g++ and clang++ versions. So if you could be so kind to share more details than the four words you graced us with I would much appreciated it.

@MichaelChirico
Copy link
Contributor Author

@eddelbuettel
Copy link
Member

eddelbuettel commented Aug 3, 2023

I don't think we say anywhere that is relevant, do we?

This pretty much a textbook case of why I say please file an issue first.

@eddelbuettel
Copy link
Member

I got nuttin' against 'include-what-you-use' and other helpers. Then again this is Rcpp and we had Rcpp.h as a portmanteau include for 1 1/2 decades. So 🤷‍♂️.

Lastly in the guidelines you ignored we also ask if you could be so kind to include a ChangeLog entry.

@eddelbuettel
Copy link
Member

On balance I am in favour, header etiquette and explicit use is better in the long run but once again we do ask that folks play by the book and you would have wasted less of your and my time if you had done so. Ah well.

@MichaelChirico
Copy link
Contributor Author

I spot-checked the files and indeed there's no direct #include for the respective objects (attributes.cpp -> std::enl, date.cpp -> TRUE,FALSE), so it seemed pretty straightforward that we should avoid transitive inclusion.

I also recall "recently" your accepting a similar PR:

eddelbuettel/rcppcctz#42

Sorry about the ChangeLog, I'm guilty of assuming such small changes don't need a log entry. My mistake :)

@eddelbuettel
Copy link
Member

Your link points at (bleeding edge?) clang-18. Is that what your toolchain had?

I seem to have clang-include-fixer-15 here (Ubuntu 23.04). Can you (just so we have the notes and could replicate) share the exact invocation of the exact tools you used (unless it violates state secrets of two dozen country and / or your likely paranoid work rules).

@MichaelChirico
Copy link
Contributor Author

Can you (just so we have the notes and could replicate) share the exact invocation of the exact tools you used

Good and fair question! Unfortunately I have no idea how to map between what I see (a CI warning) and an externally-visible version / command-line invocation that would reproduce it. clang-18 looks correct at a glance. Let me ask around a bit.

@eddelbuettel
Copy link
Member

Thanks -- using these tools is a net-positive, and it good that Google does (or did, at least) sponsor llvm and other tooling. But to me really useful to selfish me I need to see how it is running. You can't expect me to read a man page, can you?

@eddelbuettel
Copy link
Member

Also formal 'finders fee kudos' for the one-char fix of what must be a years old typo. I'll overlook the fact that you are mixing issues 😀 because one char does not warrant another PR.

All good, and thanks!

@eddelbuettel eddelbuettel merged commit f46b724 into RcppCore:master Aug 3, 2023
7 checks passed
@MichaelChirico MichaelChirico deleted the master-1 branch August 3, 2023 18:37
@MichaelChirico
Copy link
Contributor Author

one char does not warrant another PR.

I should hope not!

@MichaelChirico
Copy link
Contributor Author

OK here we go:

The misc-include-cleaner check was added in June:

https://github.com/llvm/llvm-project/commits/main/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h

The tool I'm seeing is a gussied-up version of this:

https://clang.llvm.org/extra/clang-tidy/#id2

@eddelbuettel
Copy link
Member

Thanks. I do have clang-tidy-15 here too but I presume I have to wait for misc-include-cleaner then.

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.

2 participants