Skip to content

Conversation

@bkietz
Copy link
Collaborator

@bkietz bkietz commented Aug 6, 2020

Adds a convenience make format target to call clang-format on **/*.hpp. Checks for clang-format-10 specifically to avoid the subtle differences between clang-format versions.

Concerns:

  • I'd like to run this on **/*.cpp as well, but cpp11test/src/cpp11.cpp is generated and I'm not sure formatting it is appropriate. Also cpp11test/R/cpp11-bindings.cpp would match that glob and is also generated but it seems it should be named cpp11test/R/cpp11-bindings.R? Generated code is marked with // clang-format off
  • I've used make conditionals and find to implement globbing, version checking, etc. It might be preferable to move that logic to an R script
  • We'd probably like to add the same functionality for the R source using formatR or similar

@bkietz bkietz requested a review from jimhester August 6, 2020 12:41
@bkietz bkietz force-pushed the clang-format-always branch from 61791f2 to 2797ff4 Compare August 6, 2020 12:42
@jimhester
Copy link
Member

Should this use run-clang-format.py? (https://github.com/Sarcasm/run-clang-format#how-to-use) it supports a .clang-format-ignore file to exclude files.

Alternatively (and maybe a better suggestion) is maybe we should just add a // clang-format off comment in the generated code

@jimhester
Copy link
Member

jimhester commented Aug 6, 2020

I think I will add //clang format off to the generated code, it is the simplest solution.

EDIT done in 0e0442d

@jimhester
Copy link
Member

Looks good, even picked up an issue :)

I will merge this and fix up the formatting issue after the merge

Thanks!

@jimhester jimhester merged commit c783cda into r-lib:master Aug 6, 2020
@jimhester jimhester deleted the clang-format-always branch August 6, 2020 16:37
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