Address clang-tidy-19 warnings

[hebasto]

This PR continues the work from #83 and addresses new warnings for clang-tidy-19:

$ cmake -B build -DCMAKE_CXX_COMPILER=clang++-19 -DCMAKE_CXX_CLANG_TIDY=clang-tidy-19
$ cmake --build build --target all mptests mpexamples

All generated files, including those produced by the target_capnp_sources() function, are no longer subject to any CMake's linting process (CMake 3.27 or later is required; however, this should be a reasonable expectation for environments where linting is performed).

[purpleKarrot]

What is the rationale for disabling some of the checks? Are fixes postponed for follow up work or is there another reason to keep them?

[hebasto]

What is the rationale for disabling some of the checks? Are fixes postponed for follow up work or is there another reason to keep them?

The performance-avoid-endl check affects only code in examples. I see no reason to churn it solely for a marginal performance gain.

The misc-use-anonymous-namespace check is questionable on its own. There have been multiple long discussions in the Bitcoin Core repo, our IRC channel, and on C++ forums in general. I’d prefer not to enforce a particular style on developers. That said, I personally prefer anonymous namespaces.

[purpleKarrot]

ACK

The performance-avoid-endl check affects only code in examples. I see no reason to churn it solely for a marginal performance gain.

It is not only about performance. It also sends a signal to readers that std::endl is not welcome.

[vasild]

I confirm that with this branch there are no warnings with clang 18, 19 and 20.

Reviewed to (including):
5a15744 clang-tidy: Suppress bugprone-empty-catch check warning

[vasild]

Reverting commit

1cc0bf5 clang-tidy: Fix bugprone-crtp-constructor-accessibility check

does not reveal any warnings, as if that commit is not needed?

[hebasto]

With the reverted commit on Ubuntu 24.10:

$ cmake -B build -DCMAKE_CXX_COMPILER=clang++-19 -DCMAKE_CXX_CLANG_TIDY=clang-tidy-19
$ cmake --build build --target all mptests mpexamples
[48/59] Building CXX object CMakeFiles/multiprocess.dir/src/mp/proxy.cpp.o
/home/hebasto/git/libmultiprocess/include/mp/proxy-types.h:301:8: warning: the implicit default constructor of the CRTP is publicly accessible; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]
  301 | struct IterateFieldsHelper
      |        ^
  302 | {
      |  
[59/59] Linking CXX executable test/mptest

Here are Clang details:

$ clang++-19 -v
Ubuntu clang version 19.1.1 (1ubuntu1)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/lib/llvm-19/bin
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/11
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/12
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/13
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/14
Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/14
Candidate multilib: .;@m64
Selected multilib: .;@m64

[vasild]

Thanks for posting the exact commands (I should have posted mine in the first place). It turns out that if the build directory is outside of the source directory, then clang-tidy does not find its config file. This is strange because it should find it in the parent directory of the source file. Anyway, I guess you can reproduce this:

cmake -B /tmp/build -DCMAKE_CXX_COMPILER=clang++-19 -DCMAKE_CXX_CLANG_TIDY=clang-tidy-19
cmake --build /tmp/build --target all mptests mpexamples

and observe no warnings from clang-tidy.

This resolves the problem:

--- i/CMakeLists.txt
+++ w/CMakeLists.txt
@@ -18,13 +18,13 @@ find_package(Threads REQUIRED)
 option(Libmultiprocess_ENABLE_CLANG_TIDY "Run clang-tidy with the compiler." OFF)
 if(Libmultiprocess_ENABLE_CLANG_TIDY)
   find_program(CLANG_TIDY_EXECUTABLE NAMES clang-tidy)
   if(NOT CLANG_TIDY_EXECUTABLE)
     message(FATAL_ERROR "Libmultiprocess_ENABLE_CLANG_TIDY is ON but clang-tidy is not found.")
   endif()
-  set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}")
+  set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}" --config-file=${CMAKE_SOURCE_DIR}/.clang-tidy)
 endif()
 
 include("cmake/compat_config.cmake")
 include("cmake/pthread_checks.cmake")
 include(GNUInstallDirs)

if -DLibmultiprocess_ENABLE_CLANG_TIDY=ON is to be used. Or if CMAKE_CXX_CLANG_TIDY is to be set directly on the command line, then it should be

cmake -B /tmp/build -DCMAKE_CXX_COMPILER=clang++19 -DCMAKE_CXX_CLANG_TIDY="clang-tidy19;--config-file=/path_to_source/libmultiprocess/.clang-tidy"

---

"config file not found when out of source" is out of the scope of this PR, so I guess this issue can be marked as resolved. I will be using a subdirectory of the source to test this PR.

[vasild]

From https://clang.llvm.org/extra/clang-tidy/checks/bugprone/empty-catch.html

To avoid these issues, developers should always handle exceptions properly

This warning looks legit. I wonder if it can be resolved instead of suppressed.

I am unable to reproduce it, so I am probably doing something wrong. But this should resolve it, suggested from capnproto/capnproto#553 (comment):

--- i/include/mp/util.h
+++ w/include/mp/util.h
@@ -6,12 +6,13 @@
 #define MP_UTIL_H
 
 #include <capnp/schema.h>
 #include <cstddef>
 #include <functional>
 #include <future>
+#include <kj/debug.h>
 #include <kj/common.h>
 #include <kj/exception.h>
 #include <kj/string-tree.h>
 #include <memory>
 #include <string.h>
 #include <string>
@@ -154,13 +155,14 @@ struct DestructorCatcher
     T value;
     template <typename... Params>
     DestructorCatcher(Params&&... params) : value(kj::fwd<Params>(params)...)
     {
     }
     ~DestructorCatcher() noexcept try {
-    } catch (const kj::Exception& e) { // NOLINT(bugprone-empty-catch)
+    } catch (const kj::Exception& e) {
+        KJ_LOG(ERROR, e);
     }
 };
 
 //! Wrapper around callback function for compatibility with std::async.
 //!
 //! std::async requires callbacks to be copyable and requires noexcept

Further, unrelated to this PR, the DestructorCatcher does not seem to work in the first place:

class A
{
public:
    ~A()
    {
        throw std::runtime_error{"aaa"};
    }
};

template <typename T>
struct DestructorCatcher
{
    T value;
    template <typename... Params>
        DestructorCatcher(Params&&... params) : value(std::forward<Params>(params)...)
    {
    }
    ~DestructorCatcher() noexcept try {
    } catch (const std::runtime_error& e) {
        // Does not work with or without a return.
        //return;
    }
};

int main(int, char**)
{
    try {
        std::shared_ptr<DestructorCatcher<A>> p{std::make_shared<DestructorCatcher<A>>()};
    } catch (const std::runtime_error& e) {
        std::cout << "in main: " << e.what() << "\n";
    }
    return 0;
}

  * frame #0: 0x0000000825ade18a libc.so.7`__sys_thr_kill at thr_kill.S:4
    frame #1: 0x0000000825a575a4 libc.so.7`__raise(s=6) at raise.c:50:10
    frame #2: 0x0000000825b0ab29 libc.so.7`abort at abort.c:64:8
    frame #3: 0x0000000000203c0e t`__clang_call_terminate + 14
    frame #4: 0x0000000000204e09 t`A::~A(this=0x00000e1f0581a038) at t.cc:184:15
    frame #5: 0x0000000000204db5 t`DestructorCatcher<A>::~DestructorCatcher(this=0x00000e1f0581a038) at t.cc:197:5
    frame #7: 0x0000000000204d59 t`void std::__1::allocator_traits<std::__1::allocator<DestructorCatcher<A>>>::destroy[abi:de180100]<DestructorCatcher<A>, void, void>((null)=0x00000008205e8697, __p=0x00000e1f0581a038) at allocator_traits.h:316:5
    frame #12: 0x0000000000203bf8 t`std::__1::shared_ptr<DestructorCatcher<A>>::~shared_ptr[abi:de180100](this=0x00000008205e8770) at shared_ptr.h:648:17
    frame #13: 0x0000000000203aaa t`main((null)=1, (null)=0x00000008205e8818) at t.cc:206:5

[ryanofsky]

re: #144 (comment)

Good catch. The workaround suggested capnproto/capnproto#553 (comment) is not implemented correctly here. The derived constructor is trying to catch exceptions from a base constructor which will not even run until after it exits. To be implemented correctly it would have to add T as a member instead of deriving from it, and either set T to null or make it a std::optional member to make sure its destruct wont throw after DestructorCatcher exits.

But I think a better fix for this issue is to change EventLoop::post method to use kj::Function instead of std::function so DestructorCatcher and AsyncCallable classes can just be deleted.

[vasild]

Now that I can reproduce, I confirm that the above patch which adds KJ_LOG() silences the warning and I think that is better than suppressing.

@ryanofsky, I am confused. Do you mean "destructor" instead of "constructor"? Also, there is no inheritance here, so there is no derived constructor. I tried to change T value; in the standalone prog above to std::unique_ptr<T> value;, std::optional<T> value; or to T* value; and changed the ~DestructorCatcher() to:

      ~DestructorCatcher() noexcept
      {
          try {
              delete value;
          } catch (const std::runtime_error& e) {
              std::cout << e.what() << "\n";
          }
      }

but it keeps crashing.

But I think a better fix for this issue is to change EventLoop::post method to use kj::Function

👍

[vasild]

... change EventLoop::post method to use kj::Function ...

Quick attempt at doing that, I am not sure how to resolve the compilation error in sync():

[patch] Remove DestructorCatcher and AsyncCallable

diff --git i/include/mp/proxy-io.h w/include/mp/proxy-io.h
index 4430a42..14d5906 100644
--- i/include/mp/proxy-io.h
+++ w/include/mp/proxy-io.h
@@ -8,12 +8,13 @@
 #include <mp/proxy.h>
 #include <mp/util.h>
 
 #include <mp/proxy.capnp.h>
 
 #include <capnp/rpc-twoparty.h>
+#include <kj/function.h>
 
 #include <assert.h>
 #include <functional>
 #include <optional>
 #include <map>
 #include <memory>
@@ -141,20 +142,22 @@ public:
     //! called once from the m_thread_id thread. This will block until
     //! the m_num_clients reference count is 0.
     void loop();
 
     //! Run function on event loop thread. Does not return until function completes.
     //! Must be called while the loop() function is active.
-    void post(const std::function<void()>& fn);
+    void post(kj::Function<void()>& fn);
 
     //! Wrapper around EventLoop::post that takes advantage of the
     //! fact that callable will not go out of scope to avoid requirement that it
     //! be copyable.
     template <typename Callable>
     void sync(Callable&& callable)
     {
+        // XXX
+        // error: no viable conversion from 'reference_wrapper<(lambda at libmultiprocess/include/mp/proxy-io.h:427:43)>' to 'kj::Function<void ()>'
         return post(std::ref(callable));
     }
 
     //! Start asynchronous worker thread if necessary. This is only done if
     //! there are ProxyServerBase::m_impl objects that need to be destroyed
     //! asynchronously, without tying up the event loop thread. This can happen
@@ -192,13 +195,13 @@ public:
 
     //! Handle of an async worker thread. Joined on destruction. Unset if async
     //! method has not been called.
     std::thread m_async_thread;
 
     //! Callback function to run on event loop thread during post() or sync() call.
-    const std::function<void()>* m_post_fn = nullptr;
+    kj::Function<void()>* m_post_fn = nullptr;
 
     //! Callback functions to run on async thread.
     CleanupList m_async_fns;
 
     //! Pipe read handle used to wake up the event loop thread.
     int m_wait_fd = -1;
diff --git i/include/mp/type-context.h w/include/mp/type-context.h
index 7c12afe..40d7cc1 100644
--- i/include/mp/type-context.h
+++ w/include/mp/type-context.h
@@ -61,13 +61,13 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
 {
     const auto& params = server_context.call_context.getParams();
     Context::Reader context_arg = Accessor::get(params);
     auto future = kj::newPromiseAndFulfiller<typename ServerContext::CallContext>();
     auto& server = server_context.proxy_server;
     int req = server_context.req;
-    auto invoke = MakeAsyncCallable(
+    auto invoke =
         [fulfiller = kj::mv(future.fulfiller),
          call_context = kj::mv(server_context.call_context), &server, req, fn, args...]() mutable {
                 const auto& params = call_context.getParams();
                 Context::Reader context_arg = Accessor::get(params);
                 ServerContext server_context{server, call_context, req};
                 bool disconnected{false};
@@ -140,13 +140,13 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
                 {
                     server.m_context.connection->m_loop.sync([&]() {
                         auto fulfiller_dispose = kj::mv(fulfiller);
                         fulfiller_dispose->reject(kj::mv(*exception));
                     });
                 }
-            });
+            };
 
     // Lookup Thread object specified by the client. The specified thread should
     // be a local Thread::Server object, but it needs to be looked up
     // asynchronously with getLocalServer().
     auto thread_client = context_arg.getThread();
     return server.m_context.connection->m_threads.getLocalServer(thread_client)
diff --git i/include/mp/util.h w/include/mp/util.h
index 0569c44..4fdfd4e 100644
--- i/include/mp/util.h
+++ w/include/mp/util.h
@@ -6,12 +6,13 @@
 #define MP_UTIL_H
 
 #include <capnp/schema.h>
 #include <cstddef>
 #include <functional>
 #include <future>
+#include <kj/debug.h>
 #include <kj/common.h>
 #include <kj/exception.h>
 #include <kj/string-tree.h>
 #include <memory>
 #include <string.h>
 #include <string>
@@ -143,52 +144,12 @@ template <typename Lock, typename Callback>
 void Unlock(Lock& lock, Callback&& callback)
 {
     UnlockGuard<Lock> unlock(lock); // NOLINT(misc-const-correctness)
     callback();
 }
 
-//! Needed for libc++/macOS compatibility. Lets code work with shared_ptr nothrow declaration
-//! https://github.com/capnproto/capnproto/issues/553#issuecomment-328554603
-template <typename T>
-struct DestructorCatcher
-{
-    T value;
-    template <typename... Params>
-    DestructorCatcher(Params&&... params) : value(kj::fwd<Params>(params)...)
-    {
-    }
-    ~DestructorCatcher() noexcept try {
-    } catch (const kj::Exception& e) { // NOLINT(bugprone-empty-catch)
-    }
-};
-
-//! Wrapper around callback function for compatibility with std::async.
-//!
-//! std::async requires callbacks to be copyable and requires noexcept
-//! destructors, but this doesn't work well with kj types which are generally
-//! move-only and not noexcept.
-template <typename Callable>
-struct AsyncCallable
-{
-    AsyncCallable(Callable&& callable) : m_callable(std::make_shared<DestructorCatcher<Callable>>(std::move(callable)))
-    {
-    }
-    AsyncCallable(const AsyncCallable&) = default;
-    AsyncCallable(AsyncCallable&&) = default;
-    ~AsyncCallable() noexcept = default;
-    ResultOf<Callable> operator()() const { return (m_callable->value)(); }
-    mutable std::shared_ptr<DestructorCatcher<Callable>> m_callable;
-};
-
-//! Construct AsyncCallable object.
-template <typename Callable>
-AsyncCallable<std::remove_reference_t<Callable>> MakeAsyncCallable(Callable&& callable)
-{
-    return std::move(callable);
-}
-
 //! Format current thread name as "{exe_name}-{$pid}/{thread_name}-{$tid}".
 std::string ThreadName(const char* exe_name);
 
 //! Escape binary string for use in log so it doesn't trigger unicode decode
 //! errors in python unit tests.
 std::string LogEscape(const kj::StringTree& string);
diff --git i/src/mp/proxy.cpp w/src/mp/proxy.cpp
index ca094e3..5b2fdf6 100644
--- i/src/mp/proxy.cpp
+++ w/src/mp/proxy.cpp
@@ -19,12 +19,13 @@
 #include <future>
 #include <kj/async-io.h>
 #include <kj/async.h>
 #include <kj/common.h>
 #include <kj/debug.h>
 #include <kj/exception.h>
+#include <kj/function.h>
 #include <kj/memory.h>
 #include <map>
 #include <memory>
 #include <mutex>
 #include <stddef.h>
 #include <stdexcept>
@@ -215,13 +216,13 @@ void EventLoop::loop()
     KJ_SYSCALL(::close(post_fd));
     std::unique_lock<std::mutex> lock(m_mutex); // NOLINT(misc-const-correctness)
     m_wait_fd = -1;
     m_post_fd = -1;
 }
 
-void EventLoop::post(const std::function<void()>& fn)
+void EventLoop::post(kj::Function<void()>& fn)
 {
     if (std::this_thread::get_id() == m_thread_id) {
         fn();
         return;
     }
     std::unique_lock<std::mutex> lock(m_mutex);

[ryanofsky]

Thanks for doing ahead with this!

•    // error: no viable conversion from 'reference_wrapper<(lambda at libmultiprocess/include/mp/proxy-io.h:427:43)>' to 'kj::Function<void ()>'
     return post(std::ref(callable));
  

This is just what comes to mind but you might be able to replace std::ref with kj::mv. Intent of std::ref is just to avoid a copy.

[ryanofsky]

Now that I can reproduce, I confirm that the above patch which adds KJ_LOG() silences the warning and I think that is better than suppressing.

I'd slightly prefer to explicitly suppress the error so we can know to revisit and fix this later than to add KJ_LOG in code we know is broken and will never run.

@ryanofsky, I am confused. Do you mean "destructor" instead of "constructor"? Also, there is no inheritance here, so there is no derived constructor.

Ooops, I'm not sure what I was looking at when I saw inheritance. And I did mean destructor.

I'm not sure about what was causing crashes you were seeing, but I if we wanted to implement suggested workaround correctly, easiest way would probably be to make member std::optional<T> value and call value.reset() in the try block.

[vasild]

I'm not sure about what was causing crashes you were seeing, but I if we wanted to implement suggested workaround correctly, easiest way would probably be to make member std::optional value and call value.reset() in the try block.

That does not work. Probably for the same reason DestructorCatcher is needed in the first place - shared_ptr, unique_ptr, optional do not like containing objects whose destructors throw. It crashes with a plain pointer and delete as well.

Try it yourself: gotbolt

[ryanofsky]

Interesting. In that case could maybe an appealing alternative could be declare T in inside an anonymous union and expliict placement-new to call constructor, and explicit call to its destructor in a try/catch. An approach like that might be able to work if can't get rid of asynccallable class, though I'm still hoping we could get rid of it using kj::Function

[ryanofsky]

re: #144 (comment)

[patch] Remove DestructorCatcher and AsyncCallable

This patch is now part of #160

[ryanofsky]

Code review ACK 19d7537. Looks great, thanks for fixing all these errors and packaging up the changes so nicely. I started fixing some of these myself, but did not get very far and you did a nicer job.

There are a number of things that could be followed up on here, mentioned in comments. But it should be easy to remaining issues by searching for nolint or modifying .clang-tidy file to include more warnings.

[ryanofsky]

In commit "cmake: Skip linting for generated sources" (a692038)

I think my preference would be to not skip linting for generated files, and I implemented changes locally to actually fix the lint errors, so I think it would be slightly better to drop this commit to avoid having to undo this later. This commit also conflicts with #142 so that could be another reason to drop it. Ok to keep though if you prefer. Just pointing out that this may change later.

[ryanofsky]

re: #144 (comment)

Good catch. The workaround suggested capnproto/capnproto#553 (comment) is not implemented correctly here. The derived constructor is trying to catch exceptions from a base constructor which will not even run until after it exits. To be implemented correctly it would have to add T as a member instead of deriving from it, and either set T to null or make it a std::optional member to make sure its destruct wont throw after DestructorCatcher exits.

But I think a better fix for this issue is to change EventLoop::post method to use kj::Function instead of std::function so DestructorCatcher and AsyncCallable classes can just be deleted.

[ryanofsky]

In commit "clang-tidy: Fix misc-const-correctness check" (06806fe)

Maybe just declare these const? It seems more stable than adding NOLINT. Especially because if any an unlock call is made to any of these locks later the lint errors would go away but nolint comments would still be there.

[ryanofsky]

re: #144 (comment)

This is done in #152

[ryanofsky]

In commit "clang-tidy: Fix misc-include-cleaner check" (90cb5a9)

Is the include-cleaner check not compatible with IWYU? When I ran IWYU it seemed to correctly add <init.capnp.proxy.h> include. Do we know if it is expected that clang-tidy would complain about this?

Same applies to other nolint below.

[hebasto]

Is the include-cleaner check not compatible with IWYU?

They seem incompatible.

When I ran IWYU it seemed to correctly add <init.capnp.proxy.h> include.

The include-what-you-use tool built from the clang_19 branch still insist on removing #include <init.capnp.proxy.h>.

It seem both linters fail to parse a convoluted template code properly.

Additionally, IWYU suggests:

#include <kj/async.h>     // for evalLater
#include <kj/common.h>    // for fwd, mv, ctor, implicitCast, instance
#include <kj/memory.h>    // for heap

for this line:https://github.com/chaincodelabs/libmultiprocess/blob/477405eda34d923bd2ba6b3abc4c4d31db84c3ea/example/calculator.cpp#L48

[ryanofsky]

In commit "clang-tidy: Suppress performance-enum-size check warning" (19d7537)

This seems like another thing that would be nice to fix instead of nolint. These things can be done as followups since it is easy to search for nolint.

[vasild]

This works:

-enum class FooEnum : int { ONE = 1, TWO = 2, }; // NOLINT(performance-enum-size)
+enum class FooEnum : uint8_t { ONE = 1, TWO = 2, };

[ryanofsky]

re: #144 (comment)

This is done in #152

[fanquake]

It'd be good if the commit messages explained what they are doing, and why. The choice to suppress or fix seems completely arbitrary here, and where suppress is done, there's no explanation why.

[ryanofsky]

The choice to suppress or fix seems completely arbitrary here, and where suppress is done, there's no explanation why.

I just assumed choice to suppress or fix was to make libmultiprocess tidy option match bitcoin tidy options? But maybe this is not the case

[hebasto]

@ryanofsky

Looks great, thanks for fixing all these errors and packaging up the changes so nicely.

Thank you!

I started fixing some of these myself, but did not get very far and you did a nicer job.

There are a number of things that could be followed up on here, mentioned in comments.

Do you mind stepping in and continuing?

[ryanofsky]

Do you mind stepping in and continuing?

Sure, I'd be happy to merge this PR as-is. It just has a conflict that nees to be resolved. IMO it just be good to drop the first commit a692038 which is causing the conflict, and since I think it would be better to keep checking generated files.

Just let me know what you'd prefer: (1) You drop commit or fix conflict here and I merge this PR? (2) I update your branch (if I have permission) and merge it? (3) We close and open a new PR?

[hebasto]

(3) We close and open a new PR?

I'll be happy to review and test it.

[vasild]

f159c75 clang-tidy: Fix readability-avoid-return-with-void-value check

There is similar, (undetected?) code in include/mp/proxy-io.h in EventLoop::sync() which warrants:

  void sync(Callable&& callable)
  {
-      return post(std::ref(callable));
+      post(std::ref(callable));
  }

[ryanofsky]

re: #144 (comment)

This is done in #152

[vasild]

This works:

-enum class FooEnum : int { ONE = 1, TWO = 2, }; // NOLINT(performance-enum-size)
+enum class FooEnum : uint8_t { ONE = 1, TWO = 2, };

[vasild]

There is one warning that is not fixed by this PR:

libmultiprocess/test/mp/test/foo.h:71:9: warning: method 'callbackSaved' can be made const [readability-make-member-function-const]
   71 |     int callbackSaved(int arg) { return m_callback->call(arg); }
      |         ^
      |                                const

[ryanofsky]

Thanks vasild! If you have any interest in opening a PR, would be happy to review it. Also fine if not, all the information you've gathered here will already be helpful to follow up.

[ryanofsky]

re: bitcoin/bitcoin#30975 (comment)

Still planning on cleaning up the warnings anyway since I already fixed a lot of them locally.

I've just read it. My apologies for overstepping.

Just saw this message. Definitely appreciate this and your other PRs. They have all be really helpful and not overstepping in any way.