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

Deleting the default constructor of some classes #27

Open
AntoineFroger opened this issue Mar 18, 2021 · 3 comments
Open

Deleting the default constructor of some classes #27

AntoineFroger opened this issue Mar 18, 2021 · 3 comments

Comments

@AntoineFroger
Copy link

For the following classes, it might make sense to delete the default-constructor:

  • event_attributes
  • domain_thread_range
  • domain_process_range

What is the rational for allowing the creation of an empty event attribute or a thread range with an empty event attribute? I understand that the NVTX API does not prohibit this behavior but a range or event without any attribute will be meaningless for most, if not all, tools.

@jrhemstad
Copy link
Contributor

For event_attributes, I believe the variadic constructors would fail if the default constructor were deleted.

E.g.,

  template <typename... Args>
  NVTX3_RELAXED_CONSTEXPR explicit event_attributes(category const& c, Args const&... args) noexcept
    : event_attributes(args...)
  {
    attributes_.category = c.get_id();
  }

When sizeof...(args) == 0, it will call the default constructor. If that were deleted, this constructor would fail. The default ctor is what ensures any unspecified field of the event attributes is default initialized.

For thread_range/process_range, I don't have strong feelings either way about deleting the default ctors. In Nsight Systems, would these just show up as an unnamed range in the NVTX swimlane? I agree thats of limited usefulness, but it's not really harmful either.

@AntoineFroger
Copy link
Author

When sizeof...(args) == 0, it will call the default constructor. If that were deleted, this constructor would fail. The default ctor is what ensures any unspecified field of the event attributes is default initialized.

I see, thanks for the explanation. If we want to discourage it, we could make the default constructor private. But that wouldn't actually prevent people from constructing an empty event_attributes object so it might not make that much sense.

For thread_range/process_range, I don't have strong feelings either way about deleting the default ctors. In Nsight Systems, would these just show up as an unnamed range in the NVTX swimlane? I agree thats of limited usefulness, but it's not really harmful either.

Nsight Systems just shows an unnamed range which has no attribute:

nsys

I think the most important question is what semantics do we want to give to a default-constructed range. Should it be an unnamed range? Or should it be an empty object that doesn't generate a range?

This intersects the discussion we have in #28. If we want to support "conditional" range (i.e. emit a range "if"), then we might need to change the semantic of the range default-constructor. Note that people that actually want to emit a range with empty attributes could still do it by explicitly creating an empty event_attributes object and passing it to the range constructor.

@jrhemstad
Copy link
Contributor

I think the most important question is what semantics do we want to give to a default-constructed range. Should it be an unnamed range? Or should it be an empty object that doesn't generate a range?

It's a good question. When I'm faced with questions like this, I usually look for inspiration in the STL.

domain_thread_range is most similar to std::lock_guard. lock_guard doesn't allow default construction.

domain_process_range is similar to std::unique_lock whose default ctor exists, but constructs with no associated mutex.

So perhaps that means domain_thread_range's default ctor should be deleted, and domain_process_ranges default ctor should exist, but not start a range.

In re: #28, we can accomplish constructing a domain_thread_range object that doesn't start a range with the tag type I described there.

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

No branches or pull requests

2 participants