Skip to content

Commit

Permalink
tests: workaround clang bug with parameter pack after default argument
Browse files Browse the repository at this point in the history
Change-Id: I69a2f9cbd5eddef9501fecc8a5438301d01ed885
  • Loading branch information
Pesa committed Jan 16, 2025
1 parent 4296fe3 commit 2b43d67
Showing 1 changed file with 10 additions and 1 deletion.
11 changes: 10 additions & 1 deletion tests/daemon/mgmt/face-manager.t.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class FaceManagerFixture : public ManagerFixtureWithAuthenticator
*/
template<typename... Args>
shared_ptr<Face>
addFace(unsigned int flags = 0, Args&&... args)
addFace(unsigned int flags, Args&&... args)
{
auto face = make_shared<DummyFace>(std::forward<Args>(args)...);
m_faceTable.add(face);
Expand Down Expand Up @@ -91,6 +91,15 @@ class FaceManagerFixture : public ManagerFixtureWithAuthenticator
return face;
}

// We cannot combine this overload with the previous one
// because clang 10 (and earlier) doesn't like it.
// This is a workaround for https://github.com/llvm/llvm-project/issues/23403
shared_ptr<Face>
addFace()
{
return addFace(0);
}

private:
template<typename T>
static void
Expand Down

3 comments on commit 2b43d67

@pulsejet
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to support clang 10?

@Pesa
Copy link
Member Author

@Pesa Pesa commented on 2b43d67 Jan 30, 2025

Choose a reason for hiding this comment

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

That's a fair question :)

The workaround is trivial in this particular case, and it's just test code, so why not? If a more elaborate workaround had been necessary, I probably wouldn't have bothered.

But to be clear: we don't have to support clang 10, this is definitely a "best effort" kind of support. I'm personally treating gcc >= 10 and clang >= 10 as the baseline because they're both ~5yo compilers (both released in H1 2020). Also they're both available out of the box on Ubuntu 20.04 which is still in widespread use. I'm planning/hoping to gradually move to C++20 over the next few months, that may require dropping a few more older compilers.

@pulsejet
Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously it's the maintainers' call for what to support, but my two cents:

  1. Backward compatibility is not free - a workaround today is technical debt tomorrow (which was newly created)
  2. 20.04 is EOL in two months, someone using it is their problem. Also they can always pin to an older version.
  3. Just because it compiles and the tests pass doesn't mean it works. IMO it's better to support one compiler and know it works rather than 5 without actually ever running the output. Unless, of course, we actually know folks using clang 10 builds.

Please sign in to comment.