This is an archive of the discontinued LLVM Phabricator instance.

[SE] Add .clang-format
ClosedPublic

Authored by jhen on Sep 12 2016, 12:24 PM.

Details

Summary

The .clang-tidy file is copied from the top-level LLVM source directory
with one change: I removed the misc-noexcept-move-constructor check
because it seemed to be triggering a lot of false positives, and I
didn't think it was necessary because StreamExecutor will usually be
compiled without exceptions enabled.

Also fix warnings generated by clang-format:

  • Moved SimpleHostPlatformDevice.h so its header include guard could have the right format.
  • Changed signatures of methods taking llvm::Twine by value to take it by const ref instead.
  • Removed a bunch of places where single-statement loops and conditionals were surrounded with braces. (This was not found by the current clang-tidy, but with a local patch that I hope to upstream soon.)

Diff Detail

Repository
rL LLVM

Event Timeline

jhen updated this revision to Diff 71041.Sep 12 2016, 12:24 PM
jhen retitled this revision from to [SE] Add .clang-format.
jhen updated this object.
jhen added reviewers: jlebar, jprice.
jhen added a subscriber: parallel_libs-commits.
jlebar edited edge metadata.Sep 12 2016, 4:02 PM

WRT the noexcept check: This is actually relevant even if SE is compiled with exceptions disabled.

If you have a moveable and copyable class with a non-noexcept move constructor and you insert it into a vector, when the vector resizes itself, it will call the copy constructor, not the move constructor. Even if you compile with -fno-exceptions.

It's nuts, I know.

Whether this is something we want to enforce in the SE code or not, I don't know. Apparently it's something we enforce in LLVM only in aspiration. :)

jlebar accepted this revision.Sep 12 2016, 4:02 PM
jlebar edited edge metadata.

LGTM as-is, or if you want to add back the noexcept check, that's also fine with me.

This revision is now accepted and ready to land.Sep 12 2016, 4:02 PM
jhen updated this revision to Diff 71216.Sep 13 2016, 12:19 PM
jhen edited edge metadata.
  • Re-add noexcept move ctor check
jhen added a comment.Sep 13 2016, 12:19 PM

I added the noexcept move constructor check back in because I learned how to get rid of false positives. It turns out that the current check won't allow you to explicitly default a noexcept constructor at the same time as you declare it. To get around this, I just moved the explicit default definitions out-of-line. Now we can keep this useful check.

In D24468#541585, @jhen wrote:

I added the noexcept move constructor check back in because I learned how to get rid of false positives. It turns out that the current check won't allow you to explicitly default a noexcept constructor at the same time as you declare it. To get around this, I just moved the explicit default definitions out-of-line. Now we can keep this useful check.

That...sort of makes sense. There is a subtle difference between =default inside the class definition, which I think means "whatever the compiler will generate by default, including not generating an implementation if it can't," and putting the =default outside the class definition, which means "provide me the default implementation of this function."

This revision was automatically updated to reflect the committed changes.
parallel-libs/trunk/streamexecutor/unittests/CoreTests/PackedKernelArgumentArrayTest.cpp