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

Perform clang-format on Azure #405

Closed
wants to merge 1 commit into from

Conversation

janisozaur
Copy link
Contributor

@janisozaur janisozaur commented Mar 28, 2019

Fixes (part of) #202.

#236 discusses the contents of .clang-format, this PR makes CI capable of enforcing it.

A problem I ran into was clang-format mistaking some of the sources for Objective-C, due to non-C++ constructs like https://github.com/Microsoft/calculator/blob/f6a6aae6e6ce1c485b4b4b02413259649cf2b58f/src/Calculator/Converters/BitFlipAutomationNameConverter.h#L13
I have no idea what that is, so I took the easy way out and only apply formatting files which name ends with .cpp.

I provided comments in the script itself, but the gist of it is:

You can see a failing build here: https://dev.azure.com/ms/calculator/_build/results?buildId=8367

And a passing one (with sources formatted) here: https://dev.azure.com/ms/calculator/_build/results?buildId=8372, as per #406

This PR does not include .clang-format, I hope @seyfer will update #236 and it will get merged.

@janisozaur
Copy link
Contributor Author

There are tools like git-clang-format that make clang-format only test relevant git commits, but it only takes 11 seconds to download the utility, perform formatting of all the sources and report it back (23s if you count whole process of getting the VM, starting agent, etc.), so I deemed it an unnecessary complication.

@rudyhuyn
Copy link
Contributor

I would suggest to use the clang-format.exe already shipped with Visual Studio 15.7 and + for consistency

@danbelcher-MSFT
Copy link

Can we set this up as a git hook so that source is automatically formatted? We want our tools to work with us and help us take contributions faster.

@janisozaur
Copy link
Contributor Author

Setting up a git hook is a client-side action.

@danbelcher-MSFT
Copy link

I see. I haven't looked into this topic and I'm new to OSS development, so this isn't an area I'm very familiar with. If this is typical process for other projects then we should follow convention.

@janisozaur
Copy link
Contributor Author

https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks

It’s important to note that client-side hooks are not copied when you clone a repository.

@seyfer
Copy link
Contributor

seyfer commented Mar 31, 2019

@janisozaur great. I will fix conflicts and review in my PR tomorrow and hope it will be merged together soon.

@ChrisGuzak
Copy link
Member

buildId=8372, as per #406

I have no idea what that is

This is a C++ CX attribute. It is not strictly needed in this case as the generated .winmd file is not exposed to javascript client code (the case where this attribute means something) so it could be removed.

I'm surprised the other CX language elements ref and the ^s don't cause problems but I'm glad clang-format accepts those.

@janisozaur
Copy link
Contributor Author

Can it be done with a regular C++ attribute?

@ChrisGuzak
Copy link
Member

Unfortunatly no. CX was invented to enable features like this that are non-standard. I recommend removing it and verifying everything works (it should).

@HowardWolosky HowardWolosky added the codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) label May 11, 2019
@mcooley
Copy link
Member

mcooley commented Feb 13, 2020

This PR has been assigned to me for almost a year, and sadly I haven't had time to dig into this and figure out the right thing to do. I'm closing this PR (and the related #406) because unfortunately I don't think that's going to change anytime soon. I'm sorry it's taken so long to come to a decision.

Having lived with .clang-format settings in the repo for a while now, the impact on development has been mixed. For the C++/CX code--which unfortunately is a lot of the code in the repo, including the code which changes most frequently--the auto-formatted results can be pretty ugly. So I think we can reconsider this change only if we reduce the amount of C++/CX code we have.

@mcooley mcooley closed this Feb 13, 2020
@janisozaur janisozaur deleted the clang-format-ci branch January 12, 2021 18:13
@janisozaur janisozaur mentioned this pull request Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants