This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Revise polly-{update|check}-format targets
ClosedPublic

Authored by Meinersbur on Sep 14 2015, 6:44 AM.

Details

Summary

Make clang-format run on each file independently using add_custom_format (instead using a shell script in utils/). The targets polly-{update|check}-format depend on these.

The primary motivation is to make them work on Windows, but also improves them generally:

  • Each file update/check can run in parallel (Although they do not take long to run anyway)
  • Implicit dependency on clang-format, so it recompiles if necessary
  • polly-check-format shows the formatting difference if failing

Diff Detail

Event Timeline

Meinersbur updated this revision to Diff 34667.Sep 14 2015, 6:44 AM
Meinersbur retitled this revision from to [Polly] Revise polly-{update|check}-format targets.
Meinersbur updated this object.
Meinersbur added reviewers: jdoerfert, grosser.
Meinersbur added a project: Restricted Project.
Meinersbur added subscribers: llvm-commits, pollydev.
grosser edited edge metadata.Sep 14 2015, 6:49 AM
grosser added a subscriber: grosser.

LGTM.

Looking at this, it would also be great if we would call polly-check-format from check-polly, if clang-format is available in our build (and we are not just a LLVM+Polly build).

Best,
Tobias

Looking at this, it would also be great if we would call polly-check-format from check-polly, if clang-format is available in our build (and we are not just a LLVM+Polly build).

In another commit.

I personally think this would be exaggerated. Some white space in the source has no influence on the correctness of build product. Also, LLVM doesn't do this either.

This revision was automatically updated to reflect the committed changes.
jdoerfert edited edge metadata.Sep 14 2015, 10:30 AM
jdoerfert added a subscriber: jdoerfert.

Meinersbur added a comment.

In http://reviews.llvm.org/D12837#245090, @grosser wrote:

Looking at this, it would also be great if we would call polly-check-format from check-polly, if clang-format is available in our build (and we are not just a LLVM+Polly build).

In another commit.

I personally think this would be exaggerated. Some white space in the source has no influence on the correctness of build product. Also, LLVM doesn't do this either.

As long as we break the buildbots with white space errors it makes sense
to break the local unit tests too. I would like such a feature if
possible.

Meinersbur added a comment.

In http://reviews.llvm.org/D12837#245090, @grosser wrote:

Looking at this, it would also be great if we would call polly-check-format from check-polly, if clang-format is available in our build (and we are not just a LLVM+Polly build).

In another commit.

I personally think this would be exaggerated. Some white space in the source has no influence on the correctness of build product. Also, LLVM doesn't do this either.

As long as we break the buildbots with white space errors it makes sense
to break the local unit tests too. I would like such a feature if
possible.

You can just run "ninja check-polly polly-check-format".

However, here is the patch: D12850

./CMakeLists.txt