Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move [[nodiscard]] and [[maybe_unused]] in front of host/device declarations #892

Merged
merged 3 commits into from
Feb 25, 2025

Conversation

StewMH
Copy link
Contributor

@StewMH StewMH commented Feb 25, 2025

Make HIP happy

@stephenswat
Copy link
Member

I like it, but can we somehow add a CI job so we don't keep reintroducing these mistakes?

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Indeed, we'd need a recipe soon about how you do these builds. So that they could be more automated.

Though, since I expect that things are not working yet, I don't think it's necessarily for this PR to sort it out.

@@ -14,7 +14,7 @@

namespace traccc {
struct [[maybe_unused]] channel0_major_cell_order_relation{
template <typename T1, typename T2> TRACCC_HOST_DEVICE [[maybe_unused]] bool
template <typename T1, typename T2>[[maybe_unused]] TRACCC_HOST_DEVICE bool
Copy link
Member

Choose a reason for hiding this comment

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

I'm not in love with the lack of a space in front of [[maybe_unused]]. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like it either but it is clang-format's preference. I think we would need to update the clang-format version used by pre-commit. If I update to v19.1.7, I get this:

/**
 * traccc library, part of the ACTS project (R&D line)
 *
 * (c) 2024 CERN for the benefit of the ACTS project
 *
 * Mozilla Public License Version 2.0
 */

#pragma once

// Project include(s).
#include "traccc/definitions/qualifiers.hpp"
#include "traccc/edm/silicon_cell_collection.hpp"

namespace traccc {
struct [[maybe_unused]] channel0_major_cell_order_relation {
    template <typename T1, typename T2>
    [[maybe_unused]] TRACCC_HOST_DEVICE bool operator()(
        const edm::silicon_cell<T1>& a, const edm::silicon_cell<T2>& b) const {
        if (a.module_index() == b.module_index()) {
            if (a.channel1() == b.channel1()) {
                return a.channel0() <= b.channel0();
            } else {
                return a.channel1() <= b.channel1();
            }
        } else {
            return true;
        }
    }
};
}  // namespace traccc

which looks nice to me, but there are lots of other changes to other files.

@StewMH
Copy link
Contributor Author

StewMH commented Feb 25, 2025

This is all stuff around the margins of @CrossR's big upcoming PR to introduce track fitting to Alpaka - once that's ready it will be tested by the current CI.

Stewart Martin-Haugh stewart.martin-haugh@stfc.ac.uk and others added 2 commits February 25, 2025 17:00
@stephenswat stephenswat enabled auto-merge (squash) February 25, 2025 17:48
@stephenswat stephenswat merged commit eb002e8 into acts-project:main Feb 25, 2025
26 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants