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

Avoid calling the wrong constructor for EDGetTokenT #31701

Merged
merged 3 commits into from Oct 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion FWCore/Utilities/interface/EDGetToken.h
Expand Up @@ -88,7 +88,8 @@ namespace edm {

//Needed to avoid EDGetTokenT(ADAPTER&&) from being called instead
// when we can use C++20 concepts we can avoid the problem using a constraint
constexpr EDGetTokenT(EDGetTokenT& iOther) noexcept : m_value{iOther.m_value} {}
constexpr EDGetTokenT(EDGetTokenT<T>& iOther) noexcept : m_value{iOther.m_value} {}
constexpr EDGetTokenT(const EDGetTokenT<T>&& iOther) noexcept : m_value{iOther.m_value} {}
Copy link
Contributor

@makortel makortel Oct 7, 2020

Choose a reason for hiding this comment

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

Would ESGetToken benefit from a similar fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed since ESGetToken doesn't support the ADAPTER ability (since it doesn't know the data product type).


constexpr EDGetTokenT& operator=(EDGetTokenT<T>& iOther) {
return (*this = const_cast<const EDGetTokenT<T>&>(iOther));
Expand Down
4 changes: 4 additions & 0 deletions FWCore/Utilities/interface/ESGetToken.h
Expand Up @@ -63,6 +63,10 @@ namespace edm {
constexpr ESGetToken<ESProduct, ESRecord>& operator=(ESGetToken<ESProduct, ESRecord>& iOther) noexcept {
return (*this = const_cast<ESGetToken<ESProduct, ESRecord> const&>(iOther));
}
constexpr ESGetToken(ESGetToken<ESProduct, ESRecord> const&& iOther) : ESGetToken(iOther) {}
constexpr ESGetToken<ESProduct, ESRecord>& operator=(ESGetToken<ESProduct, ESRecord> const&& iOther) noexcept {
return (*this = iOther);
}

constexpr unsigned int transitionID() const noexcept { return m_transitionID; }
constexpr bool isInitialized() const noexcept { return transitionID() != std::numeric_limits<unsigned int>::max(); }
Expand Down
3 changes: 0 additions & 3 deletions FWCore/Utilities/test/BuildFile.xml
Expand Up @@ -54,9 +54,6 @@
<use name="tbb"/>
</bin>

<bin file="EDGetToken_t.cpp">
</bin>

<bin file="RunningAverage_t.cpp">
<use name="tbb"/>
</bin>
51 changes: 0 additions & 51 deletions FWCore/Utilities/test/EDGetToken_t.cpp

This file was deleted.

96 changes: 96 additions & 0 deletions FWCore/Utilities/test/test_catch2_EDGetToken.cc
@@ -0,0 +1,96 @@
#include <functional>
#include "FWCore/Utilities/interface/EDGetToken.h"
#include "catch.hpp"

namespace edm {
class TestEDGetToken {
public:
template <typename... Args>
static edm::EDGetToken makeToken(Args&&... iArgs) {
return edm::EDGetToken(std::forward<Args>(iArgs)...);
}

template <typename T, typename... Args>
static edm::EDGetTokenT<T> makeTokenT(Args&&... iArgs) {
return edm::EDGetTokenT<T>(std::forward<Args>(iArgs)...);
}

template <typename T, typename... Args>
static const edm::EDGetTokenT<T> makeConstTokenT(Args&&... iArgs) {
return edm::EDGetTokenT<T>(std::forward<Args>(iArgs)...);
}
};
} // namespace edm

namespace {
class Adapter {
public:
template <typename T>
edm::EDGetTokenT<T> consumes() const {
return edm::TestEDGetToken::makeTokenT<T>(11U);
}
};
} // namespace

TEST_CASE("Test EDGetToken", "[EDGetToken]") {
SECTION("EDGetTokenT") {
SECTION("No argument ctr") {
edm::EDGetTokenT<int> token1 = edm::TestEDGetToken::makeTokenT<int>();
REQUIRE(token1.isUninitialized());
REQUIRE((token1.index() == 0xFFFFFFFF));
}
SECTION("1 arg ctr") {
edm::EDGetTokenT<int> token2 = edm::TestEDGetToken::makeTokenT<int>(11U);
REQUIRE(!token2.isUninitialized());
REQUIRE((token2.index() == 11));
}
SECTION("non const arg copy ctr") {
edm::EDGetTokenT<int> token2 = edm::TestEDGetToken::makeTokenT<int>(11U);
edm::EDGetTokenT<int> token3(token2);
REQUIRE(!token3.isUninitialized());
REQUIRE((token3.index() == 11));
}

SECTION("const arg copy ctr") {
const edm::EDGetTokenT<int> cToken2 = edm::TestEDGetToken::makeTokenT<int>(11U);
edm::EDGetTokenT<int> token4(cToken2);
REQUIRE(!token4.isUninitialized());
REQUIRE(token4.index() == 11);
}

SECTION("const arg copy ctr with move") {
auto const t = edm::TestEDGetToken::makeConstTokenT<int>(11U);
const edm::EDGetTokenT<int> cToken2{std::move(t)};
REQUIRE(!cToken2.isUninitialized());
REQUIRE(cToken2.index() == 11);
}

SECTION("Use Adapter") {
Adapter a;
const edm::EDGetTokenT<int> cToken2{a};
REQUIRE(!cToken2.isUninitialized());
REQUIRE(cToken2.index() == 11);
}
}

SECTION("EDGetToken") {
SECTION("No arg ctr") {
edm::EDGetToken token10 = edm::TestEDGetToken::makeToken();
REQUIRE(token10.isUninitialized());
REQUIRE(token10.index() == 0xFFFFFFFF);
}

SECTION("1 arg ctr") {
edm::EDGetToken token11 = edm::TestEDGetToken::makeToken(100);
REQUIRE(!token11.isUninitialized());
REQUIRE(token11.index() == 100);
}

SECTION("EDGetTokenT to EDGetToken ctr") {
edm::EDGetTokenT<int> token2 = edm::TestEDGetToken::makeTokenT<int>(11U);
edm::EDGetToken token12(token2);
REQUIRE(!token12.isUninitialized());
REQUIRE(token12.index() == 11);
}
}
}