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

src: make minor improvements to EnabledDebugList #44350

Merged

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Aug 22, 2022

  • Change the underlying type of DebugCategory to unsigned int so that it can be safely cast to an unsigned type without having to worry about negative values, removing the need for the DCHECK_GE statements.

  • To fully benefit from type safety, remove DebugCategory::CATEGORY_COUNT and instead add a constexpr kDebugCategoryCount.

  • Remove the second argument from EnabledDebugList::set_enabled() and EnabledDebugList::Parse() because it was always set to true.

@tniessen
Copy link
Member Author

cc @nodejs/cpp-reviewers

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 22, 2022
src/debug_utils.h Outdated Show resolved Hide resolved
src/debug_utils.h Outdated Show resolved Hide resolved
src/debug_utils.h Outdated Show resolved Hide resolved
Change the underlying type of DebugCategory to unsigned int so that it
can be safely cast to an unsigned type without having to worry about
negative values, removing the need for the DCHECK_GE statements.

To fully benefit from type safety, remove DebugCategory::CATEGORY_COUNT
and instead add a constexpr kDebugCategoryCount.

Remove the second argument from EnabledDebugList::set_enabled() and
EnabledDebugList::Parse() because it was always set to true.
@tniessen tniessen force-pushed the src-improve-enableddebuglist branch from 1b811f6 to 9045150 Compare August 23, 2022 15:52
@tniessen
Copy link
Member Author

I've reverted the changes to EnabledDebugList::enabled_. PTAL @addaleax @RaisinTen.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM

@tniessen tniessen added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 24, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 25, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 25, 2022
@nodejs-github-bot nodejs-github-bot merged commit ddc5da9 into nodejs:main Aug 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in ddc5da9

sidwebworks pushed a commit to sidwebworks/node that referenced this pull request Aug 26, 2022
Change the underlying type of DebugCategory to unsigned int so that it
can be safely cast to an unsigned type without having to worry about
negative values, removing the need for the DCHECK_GE statements.

To fully benefit from type safety, remove DebugCategory::CATEGORY_COUNT
and instead add a constexpr kDebugCategoryCount.

Remove the second argument from EnabledDebugList::set_enabled() and
EnabledDebugList::Parse() because it was always set to true.

PR-URL: nodejs#44350
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
Change the underlying type of DebugCategory to unsigned int so that it
can be safely cast to an unsigned type without having to worry about
negative values, removing the need for the DCHECK_GE statements.

To fully benefit from type safety, remove DebugCategory::CATEGORY_COUNT
and instead add a constexpr kDebugCategoryCount.

Remove the second argument from EnabledDebugList::set_enabled() and
EnabledDebugList::Parse() because it was always set to true.

PR-URL: #44350
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
Change the underlying type of DebugCategory to unsigned int so that it
can be safely cast to an unsigned type without having to worry about
negative values, removing the need for the DCHECK_GE statements.

To fully benefit from type safety, remove DebugCategory::CATEGORY_COUNT
and instead add a constexpr kDebugCategoryCount.

Remove the second argument from EnabledDebugList::set_enabled() and
EnabledDebugList::Parse() because it was always set to true.

PR-URL: nodejs#44350
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@juanarbol
Copy link
Member

Getting compile error in the v16.x branch.

../tools/code_cache/mkcodecache.cc:45:41: error: call to member function 'Parse' is ambiguous
  node::per_process::enabled_debug_list.Parse(nullptr);
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
../src/debug_utils.h:74:8: note: candidate function
  void Parse(std::shared_ptr<KVStore> env_vars = nullptr,
       ^
../src/debug_utils.h:79:8: note: candidate function
  void Parse(const std::string& cats);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants