[WIP] Test clang-tidy with selected checks #35

ianna · 2019-12-18T11:24:44Z

Apply fixes from clang-tidy with -checks='-*,
boost-use-to-string,
misc-string-compare,
misc-uniqueptr-reset-release,
modernize-deprecated-headers,
modernize-make-shared,
modernize-use-bool-literals,
modernize-use-equals-delete,
modernize-use-nullptr,
modernize-use-override,
performance-unnecessary-copy-initialization,
readability-container-size-empty,
readability-redundant-string-cstr,
readability-static-definition-in-anonymous-namespace,
readability-uniqueptr-delete-release'

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

@jpivarski - please, review and comment

…-string-compare,misc-uniqueptr-reset-release,modernize-deprecated-headers,modernize-make-shared,modernize-use-bool-literals,modernize-use-equals-delete,modernize-use-nullptr,modernize-use-override,performance-unnecessary-copy-initialization,readability-container-size-empty,readability-redundant-string-cstr,readability-static-definition-in-anonymous-namespace,readability-uniqueptr-delete-release'

jpivarski

It looks to me like the only changes are:

replacing virtual with override (good: I didn't know about that one, but it's C++11 and I prefer it)
container.size() == 0 with container.empty() (fine: I should have known about that, as it's not a new thing, but okay)
using make_shared: I thought I couldn't use it, thinking that it's a C++14 thing, but I was wrong. It's defined in C++11.

I also added a note about json.h because looking over these changes reminded me that I'll need to hide the dependency on RapidJSON headers. (With them included in the .h file, dependent libraries like Josh's are forced to find and include the RapidJSON headers as well. If it were only used in the .cpp file, then they would get the functionality without having to find the includes. We use visibility=hidden, so there shouldn't be an issue with conflicting symbols, if he happened to include his own different version of RapidJSON.)

I'm in favor of these changes, the best one being virtual → override. However, there are so many that I think we should coordinate when they get applied: I should merge my PR, then we run clang-tidy, then merge that. My current PR has so many changes that it would be difficult to merge this in.

In fact, I'll try making these three changes manually to my branch. Then when we apply clang-tidy on top of it, it will be easier to see if there are other changes it's trying to make that are hidden by the noise that these are making.

jpivarski · 2019-12-18T12:09:37Z

src/libawkward/array/Record.cpp

@@ -52,7 +53,7 @@ namespace awkward {
    size_t cols = (size_t)numfields();
    std::shared_ptr<RecordArray::ReverseLookup> keys = array_.reverselookup();
    if (istuple()) {
-      keys = std::shared_ptr<RecordArray::ReverseLookup>(new RecordArray::ReverseLookup);
+      keys = std::make_shared<RecordArray::ReverseLookup>();
      for (size_t j = 0;  j < cols;  j++) {


Isn't make_shared a C++14 thing?

I was wrong! This is "since C++11". I guess I should have been using this, so that there are fewer dangerous-looking new keywords around.

jpivarski · 2019-12-18T12:15:58Z

include/awkward/array/EmptyArray.h

+    bool haskey(const std::string& key) const override;
+    const std::vector<std::string> keyaliases(int64_t fieldindex) const override;
+    const std::vector<std::string> keyaliases(const std::string& key) const override;
+    const std::vector<std::string> keys() const override;


Replacing virtual with override: okay. I didn't know about that, but it provides some additional safety.

jpivarski · 2019-12-18T12:17:31Z

src/libawkward/Slice.cpp

@@ -163,7 +163,7 @@ namespace awkward {
  }

  const std::shared_ptr<SliceItem> Slice::head() const {
-    if (items_.size() != 0) {
+    if (!items_.empty()) {


I guess the intent here is more specific, so this is good.

jpivarski · 2019-12-18T12:22:47Z

include/awkward/io/json.h

-    virtual void beginrecord() { writer_.StartObject(); }
-    virtual void field(const char* x) { writer_.Key(x); }
-    virtual void endrecord() { writer_.EndObject(); }
+    void null() override { writer_.Null(); }


It's not visible in this snippet, but this file leaks a dependence on the RapidJSON headers that I'll need to hide in the .cpp file. I think that can be done by defining these ToJson classes without implementations in the header and moving all of the implementations to the .cpp file, but that's something that would have to be done manually.

ianna · 2019-12-18T12:42:12Z

ok, I'm closing this PR for now.

ianna · 2019-12-18T13:28:23Z

reopen to add clang-tidy fixes with a readability-redundant-smartptr-get check:
https://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-smartptr-get.html

jpivarski

Actually, if we could have the opposite of this, to force the inclusion of .get(), I'd prefer that. I was trying to be conscious of the difference between smart pointers and raw pointers everywhere.

ianna · 2019-12-18T13:41:19Z

automated fixes with readability-avoid-const-params-in-decls check remove const in places where it could be a reference:

diff --git a/include/awkward/Index.h b/include/awkward/Index.h
index 40f7a59..2ad904b 100644
--- a/include/awkward/Index.h
+++ b/include/awkward/Index.h
@@ -32,7 +32,7 @@ namespace awkward {
 
     const std::string classname() const;
     const std::string tostring() const;
-    const std::string tostring_part(const std::string indent, const std::string pre, const std::string post) const;
+    const std::string tostring_part(std::string indent, std::string pre, std::string post) const;
     T getitem_at(int64_t at) const;
     T getitem_at_nowrap(int64_t at) const;
     void setitem_at_nowrap(int64_t at, T value) const;

https://clang.llvm.org/extra/clang-tidy/checks/readability-avoid-const-params-in-decls.html

jpivarski · 2019-12-18T13:47:17Z

In the above example, I'm going to want to make sure that it says

const std::string tostring_part(const std::string& indent, const std::string& pre, const std::string& post) const;

I just haven't been diligent about making sure that all such cases are explicit.

Most of the function arguments are either simple numbers, which should be passed by value, or unmodified structures, which should be pass-by-const-reference. It's very rare that I'll want to copy a structure into the function (as the mistaken case above) or pass a non-const reference for remote modification (though there are a few cases, but I think they're numbers).

jpivarski · 2019-12-18T15:08:45Z

include/awkward/array/RawArray.h

-        int64_t numer = abs(start - stop);
-        int64_t denom = abs(step);
+        int64_t numer = std::abs(start - stop);
+        int64_t denom = std::abs(step);


This is good. I like this.

jpivarski · 2019-12-18T17:52:07Z

My last four commits to PR #33 applied the corrections that clang-tidy wants to make, but manually with much testing to make sure that it agrees with my intentions. If you run clang-tidy over that, we should see what residual changes it wants to make with much less noise.

This is not including the removal of .get() from shared pointers. I want to do that, as a reminder of the distinction between a shared pointer and a raw pointer. (There are a few, very rare cases where raw pointers are necessary, particularly for interacting with the bottom layer in the architecture through an "extern C" interface.)

ianna · 2019-12-18T18:17:43Z

My last four commits to PR #33 applied the corrections that clang-tidy wants to make, but manually with much testing to make sure that it agrees with my intentions. If you run clang-tidy over that, we should see what residual changes it wants to make with much less noise.

This is not including the removal of .get() from shared pointers. I want to do that, as a reminder of the distinction between a shared pointer and a raw pointer. (There are a few, very rare cases where raw pointers are necessary, particularly for interacting with the bottom layer in the architecture through an "extern C" interface.)

Thanks, @jpivarski ! I'll do it tomorrow. I've been checking other clang-tidy checks. The good news is that most of them come clean. There are two we may want to look closely: modernize-pass-by-value (https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html) and readability-avoid-const-params-in-decls

ianna requested a review from jpivarski December 18, 2019 11:25

jpivarski reviewed Dec 18, 2019

View reviewed changes

ianna closed this Dec 18, 2019

ianna reopened this Dec 18, 2019

extra clang-tidy checks readability-redundant-smartptr-get

fb15d4c

jpivarski reviewed Dec 18, 2019

View reviewed changes

ianna closed this Dec 18, 2019

ianna deleted the clang-tidy-test branch December 20, 2019 09:18

jpivarski mentioned this pull request Dec 21, 2023

ci: add clang-format to pre-commit configuration #2902

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Test clang-tidy with selected checks #35

[WIP] Test clang-tidy with selected checks #35

ianna commented Dec 18, 2019

jpivarski left a comment

jpivarski Dec 18, 2019

jpivarski Dec 18, 2019

jpivarski Dec 18, 2019

jpivarski Dec 18, 2019

jpivarski Dec 18, 2019

ianna commented Dec 18, 2019

ianna commented Dec 18, 2019

jpivarski left a comment

ianna commented Dec 18, 2019

jpivarski commented Dec 18, 2019

jpivarski Dec 18, 2019

jpivarski commented Dec 18, 2019

ianna commented Dec 18, 2019

[WIP] Test clang-tidy with selected checks #35

[WIP] Test clang-tidy with selected checks #35

Conversation

ianna commented Dec 18, 2019

jpivarski left a comment

Choose a reason for hiding this comment

jpivarski Dec 18, 2019

Choose a reason for hiding this comment

jpivarski Dec 18, 2019

Choose a reason for hiding this comment

jpivarski Dec 18, 2019

Choose a reason for hiding this comment

jpivarski Dec 18, 2019

Choose a reason for hiding this comment

jpivarski Dec 18, 2019

Choose a reason for hiding this comment

ianna commented Dec 18, 2019

ianna commented Dec 18, 2019

jpivarski left a comment

Choose a reason for hiding this comment

ianna commented Dec 18, 2019

jpivarski commented Dec 18, 2019

jpivarski Dec 18, 2019

Choose a reason for hiding this comment

jpivarski commented Dec 18, 2019

ianna commented Dec 18, 2019