DataStorm linter suggested fixes #3293

pepone · 2024-12-19T11:58:00Z

This PR:

Includes fixes for issues reported by clang-tidy.
Added [[nodiscard]] to more DataStorm APIs where it make sense to do so.
Added using namespace Ice; to DataStorm implementation.

pepone · 2024-12-19T11:59:03Z

.clang-tidy

@@ -16,8 +16,8 @@ HeaderFilterRegex: '.*'
 ExcludeHeaderFilterRegex: 'include/lmdb.h|Grammar.h'
 UseColor: true
 FormatStyle: 'file'
-ExtraArgs: ['-std=c++20']
+ExtraArgs: ['-std=c++17']


Otherwise we get lints for issues that require using C++ 20 and we are still using C++ 17 with GCC.

However, this breaks the linting of the Ice/span test. Maybe a better solution would be to disable the c++20 lints/suggestions?

Maybe we could a .clang-tidy file to this test folder with an override to enable -std=c++20. I think clang-tidy supports this.

I added a separate config for Ice/span test

pepone · 2024-12-19T12:00:16Z

cpp/include/DataStorm/DataStorm.h

-// Public template based API implementation
-//
-
-namespace DataStorm


no point in declaring the same namespace twice.

pepone · 2024-12-19T12:02:24Z

cpp/include/DataStorm/Node.h

@@ -140,7 +140,7 @@ namespace DataStorm
         * provided the Node will use the default callback executor that executes callback in a dedicated thread.
         */
        Node(
-            Ice::CommunicatorPtr communicator,
+            const Ice::CommunicatorPtr& communicator,


Because we always use it as a reference.

The signature is not / should not be about the implementation but what we signal to the caller.

Here, const& tells the caller: I am reading this communicator in this ctor but I don't keep a refcount on the communicator. Whereas, by value means "I keep a refcount".

I would expect the Node to keep a refcount on the communicator, possibly indirectly by holding an Ice::InstancePtr.

pepone · 2024-12-19T12:14:20Z

cpp/include/DataStorm/Types.h

-            : sampleCount(std::move(sampleCount)),
-              sampleLifetime(std::move(sampleLifetime)),
-              clearHistory(std::move(clearHistory))
+            : sampleCount{sampleCount},


see performance-move-const-arg for trivially-copyable

pepone · 2024-12-19T12:17:41Z

cpp/src/DataStorm/DataElementI.cpp

@@ -444,7 +446,6 @@ DataElementI::getConnectedElements() const
 {
    unique_lock<mutex> lock(_parent->_mutex);
    vector<string> elements;
-    elements.reserve(_listeners.size());


This reserve doesn't match the final size, there is an inner loop.

pepone · 2024-12-19T12:18:24Z

cpp/src/DataStorm/DataElementI.cpp

        }
-        _executor->queue([self = shared_from_this(), init, keys = std::move(keys)] { init(std::move(keys)); }, true);
+        _executor->queue([init = std::move(init), keys = std::move(keys)]() mutable { init(std::move(keys)); }, true);


Without marking the lambda as mutable, the move keys doesn't have any effect.

pepone · 2024-12-19T12:24:55Z

cpp/src/DataStorm/ForwarderManager.h

-                    Response response,
-                    Exception exception,
+                    std::function<void(bool, const Ice::ByteSeq&)> response,
+                    std::function<void(std::exception_ptr)> exception,


I added std::function to performance-unnecessary-value-param.AllowedTypes

But this doesn't work with the alias.

I added std::function to performance-unnecessary-value-param.AllowedTypes

I would not do that. When you copy of std::function that holds a lambda, you copy its capture.

We often pass std::function by value because we move them around. In some cases, it's ok to pass them by const& as clang-tidy suggests.

reverted this, and fixed the forwarder manager to use pass by reference.

pepone · 2024-12-19T12:26:21Z

cpp/src/DataStorm/Instance.h

        {
            std::unique_lock<std::mutex> lock(_mutex);
            if (_timer)
            {
-                _timer->schedule(std::move(task), delay);
+                _timer->schedule(task, delay);


task is a const reference

pepone · 2024-12-19T12:28:00Z

cpp/src/DataStorm/NodeI.cpp

      // The subscriber and publisher collocated forwarders are initalized here to avoid using a nullable proxy. These
      // objects are only used after the node is initialized and are removed in destroy implementation.
      _publisherForwarder{instance->getCollocatedForwarder()->add<PublisherSessionPrx>(
-          [this](Ice::ByteSeq inParams, const Ice::Current& current) { forwardToPublishers(inParams, current); })},
+          [this](const ByteSeq& inParams, const Current& current) { forwardToPublishers(inParams, current); })},


The forwarding methods take a const reference for inParams.

pepone · 2024-12-19T12:32:10Z

cpp/src/DataStorm/NodeSessionI.cpp

            NodePrx node)
            : _nodeSessionManager(std::move(nodeSessionManager)),
-              _nodeSession(std::move(nodeSession)),
+              _nodeSession(nodeSession),


_nodeSession is a weak_ptr. The weak_ptr constructor takes a const reference to a shared_ptr.

pepone · 2024-12-19T12:37:29Z

cpp/src/DataStorm/SessionI.cpp

@@ -477,7 +475,7 @@ SessionI::initSamples(int64_t topicId, DataSamplesSeq samplesSeq, const Ice::Cur

                    vector<shared_ptr<Sample>> samplesI;
                    samplesI.reserve(samples.samples.size());
-                    auto sampleFactory = topic->getSampleFactory();
+                    const auto& sampleFactory = topic->getSampleFactory();


getSampleFactory returns a const reference.

pepone · 2024-12-19T12:40:01Z

cpp/src/DataStorm/SessionI.cpp

@@ -984,30 +982,30 @@ SessionI::unsubscribeFromKey(
    int64_t keyId)
 {
    assert(_topics.find(topicId) != _topics.end());
-    auto& subscriber = _topics.at(topicId).getSubscriber(element->getTopic());
-    auto k = subscriber.get(elementId);
-    if (k)


Not lint related, using auto here was making the code harder to read.

pepone · 2024-12-19T12:44:15Z

cpp/src/DataStorm/TraceUtil.h

-        }
-    };
-
-    class Warning : public Ice::Warning


The Warning class was never used.

pepone · 2024-12-19T12:45:30Z

cpp/test/DataStorm/api/Writer.cpp

            reader.waitForUnread(0);
            [[maybe_unused]] bool hasUnread = reader.hasUnread();
-            if (false)


I don't see the point of this if (false), this API test is kind of weird with lots of unused variables.

I am surprised the compiler doesn't complain.

bernardnormier

Looks good except for the addition of std::function to performance-unnecessary-value-param.AllowedTypes.

bernardnormier · 2024-12-19T13:21:33Z

cpp/include/DataStorm/Node.h

@@ -140,7 +140,7 @@ namespace DataStorm
         * provided the Node will use the default callback executor that executes callback in a dedicated thread.
         */
        Node(
-            Ice::CommunicatorPtr communicator,
+            const Ice::CommunicatorPtr& communicator,


The signature is not / should not be about the implementation but what we signal to the caller.

Here, const& tells the caller: I am reading this communicator in this ctor but I don't keep a refcount on the communicator. Whereas, by value means "I keep a refcount".

I would expect the Node to keep a refcount on the communicator, possibly indirectly by holding an Ice::InstancePtr.

bernardnormier · 2024-12-19T13:23:27Z

cpp/src/DataStorm/Node.cpp

 {
 }

 Node::Node(
-    Ice::CommunicatorPtr communicator,
+    const CommunicatorPtr& communicator,


I assume _instance below keeps a refcount on communicator, so it should be by value. You may want to fix the DataStormI::Instance ctor - you actually already fixed it!

bernardnormier · 2024-12-19T13:24:58Z

cpp/test/DataStorm/api/Writer.cpp

            reader.waitForUnread(0);
            [[maybe_unused]] bool hasUnread = reader.hasUnread();
-            if (false)


I am surprised the compiler doesn't complain.

bernardnormier · 2024-12-19T13:29:17Z

cpp/src/DataStorm/TopicI.h

@@ -28,7 +28,7 @@ namespace DataStormI
    public:
        TopicI(
            std::shared_ptr<Instance>,
-            std::shared_ptr<TopicFactoryI>,
+            const std::shared_ptr<TopicFactoryI>&,


Here, switching to const shared_ptr& makes sense: we don't adopt a refcount but create and keep a weak_ptr from this const shared_ptr&.

externl

Looks good!

externl · 2024-12-19T13:45:03Z

.clang-tidy

@@ -16,8 +16,8 @@ HeaderFilterRegex: '.*'
 ExcludeHeaderFilterRegex: 'include/lmdb.h|Grammar.h'
 UseColor: true
 FormatStyle: 'file'
-ExtraArgs: ['-std=c++20']
+ExtraArgs: ['-std=c++17']


Maybe we could a .clang-tidy file to this test folder with an override to enable -std=c++20. I think clang-tidy supports this.

…oc-ice/ice#3293)

pepone added 3 commits December 19, 2024 12:50

DataStorm fixes for C++ lints, nodiscard, and Ice qualification

1d1f89b

Merge remote-tracking branch 'origin/main' into datastorm-lint-fixes1

94dbd86

cleanup clang-tidy

331a640

pepone requested review from externl and bernardnormier December 19, 2024 11:58

pepone commented Dec 19, 2024

View reviewed changes

cpp/src/DataStorm/TraceUtil.h

}

};

class Warning : public Ice::Warning

Copy link

Member Author

pepone Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Warning class was never used.

pepone commented Dec 19, 2024

View reviewed changes

bernardnormier approved these changes Dec 19, 2024

View reviewed changes

externl approved these changes Dec 19, 2024

View reviewed changes

Review fixes

e40bb81

pepone merged commit 50fbf7e into zeroc-ice:main Dec 19, 2024
21 checks passed

InsertCreativityHere pushed a commit to InsertCreativityHere/compiler-comparison that referenced this pull request Jan 1, 2025

HEAD:(zeroc-ice/ice@50fbf7e95f) DataStorm linter suggested fixes (zer…

367c05c

…oc-ice/ice#3293)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataStorm linter suggested fixes #3293

DataStorm linter suggested fixes #3293

pepone commented Dec 19, 2024

pepone Dec 19, 2024

bernardnormier Dec 19, 2024

externl Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024

bernardnormier Dec 19, 2024

pepone Dec 19, 2024 •

edited

Loading

pepone Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024

bernardnormier Dec 19, 2024 •

edited

Loading

pepone Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024

pepone Dec 19, 2024 •

edited

Loading

bernardnormier Dec 19, 2024

bernardnormier left a comment

bernardnormier Dec 19, 2024

bernardnormier Dec 19, 2024

bernardnormier Dec 19, 2024

bernardnormier Dec 19, 2024

externl left a comment

externl Dec 19, 2024

DataStorm linter suggested fixes #3293

DataStorm linter suggested fixes #3293

Conversation

pepone commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

externl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone Dec 19, 2024 •

edited

Loading

bernardnormier Dec 19, 2024 •

edited

Loading

pepone Dec 19, 2024 •

edited

Loading