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

Change json gtest environment variable to compile-time definition #14541

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

davidwendt
Copy link
Contributor

Description

Changes the NJP_DEBUG_DUMP environment variable used in json_tree.cpp gtest source to a local #define compile-time definition instead. This preserves the rather complex code for debugging json parsing without requiring building it otherwise.
This is part of larger work to refactor and document env vars in the libcudf code.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Dec 1, 2023
@davidwendt davidwendt self-assigned this Dec 1, 2023
@davidwendt davidwendt requested a review from a team as a code owner December 1, 2023 14:54
print_tree(gpu_tree);
}
#if NJP_DEBUG_DUMP
printf("AFTER traversal (gpu_tree):\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
printf("AFTER traversal (gpu_tree):\n");
printf("AFTER traversal (gpu_tree):\n");

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 believe this was done on purpose to line up with line 880: BEFORE output (which has 1 more letter than AFTER) .

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgive my obsession with std print alignment! 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aesthetics is to be admired not scorned.

Copy link
Contributor

Choose a reason for hiding this comment

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

I love that @davidwendt figured it out :)
We should take this opportunity to argue about "tabs vs spaces" once again :D

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I would prefer a different name that I recognize -- NJP presumably stands for "new JSON parser" but I wouldn't have guessed that without the crowd wisdom from @vuule's recent talk. Suggesting LIBCUDF_JSON_DEBUG_DUMP.

Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

Change looks good to me.
MacroNJP_DEBUG_DUMP was introduced during development of nested json parser (NJP @elstehle ) and we continued to use it.
Reasoning for still having these macros around: these macros are useful in debugging the json reader while adding new features.

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit b5f50ef into rapidsai:branch-24.02 Dec 6, 2023
67 checks passed
@davidwendt davidwendt deleted the remove-njp-debug-dump branch December 6, 2023 19:33
karthikeyann pushed a commit to karthikeyann/cudf that referenced this pull request Dec 12, 2023
…pidsai#14541)

Changes the `NJP_DEBUG_DUMP` environment variable used in `json_tree.cpp` gtest source to a local `#define` compile-time definition instead. This preserves the rather complex code for debugging json parsing without requiring building it otherwise.
This is part of larger work to refactor and document env vars in the libcudf code.

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Bradley Dice (https://github.com/bdice)
  - Karthikeyan (https://github.com/karthikeyann)

URL: rapidsai#14541
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants