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

Revert "Replace most of preprocessor usage in nvcomp adapter with constexpr" #11999

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

vuule
Copy link
Contributor

@vuule vuule commented Oct 25, 2022

Reverts #11980

The PR was made under the assumption that if constexpr branches can contain invalid code, if the branch is not taken. However, this only holds for templates.

@vuule vuule added bug Something isn't working non-breaking Non-breaking change labels Oct 25, 2022
@vuule vuule requested a review from a team as a code owner October 25, 2022 23:42
@vuule vuule self-assigned this Oct 25, 2022
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Oct 25, 2022
Copy link
Contributor

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

🥶

@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Base: 87.40% // Head: 88.15% // Increases project coverage by +0.74% 🎉

Coverage data is based on head (14477bb) compared to base (f72c4ce).
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-22.12   #11999      +/-   ##
================================================
+ Coverage         87.40%   88.15%   +0.74%     
================================================
  Files               133      133              
  Lines             21833    21995     +162     
================================================
+ Hits              19084    19389     +305     
+ Misses             2749     2606     -143     
Impacted Files Coverage Δ
python/strings_udf/strings_udf/__init__.py 86.27% <0.00%> (-10.61%) ⬇️
python/cudf/cudf/io/text.py 91.66% <0.00%> (-8.34%) ⬇️
python/cudf/cudf/core/_base_index.py 82.20% <0.00%> (-3.35%) ⬇️
python/strings_udf/strings_udf/_typing.py 94.73% <0.00%> (-1.06%) ⬇️
python/cudf/cudf/testing/dataset_generator.py 72.83% <0.00%> (-0.42%) ⬇️
python/dask_cudf/dask_cudf/backends.py 84.90% <0.00%> (-0.37%) ⬇️
python/dask_cudf/dask_cudf/core.py 73.92% <0.00%> (-0.21%) ⬇️
python/cudf/cudf/io/orc.py 92.94% <0.00%> (-0.09%) ⬇️
python/cudf/cudf/__init__.py 90.69% <0.00%> (ø)
python/cudf/cudf/utils/ioutils.py 79.47% <0.00%> (ø)
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vuule
Copy link
Contributor Author

vuule commented Oct 26, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c146d21 into branch-22.12 Oct 26, 2022
@vuule vuule deleted the revert-11980-impr-nvcomp-adapter-constexpr branch October 26, 2022 16:35
@bdice
Copy link
Contributor

bdice commented Oct 26, 2022

For posterity:

The symbols in the if constexpr body may have to exist, even if they're supposed to be compiled out. https://godbolt.org/z/h3a7vazzc

Older nvcomp versions do not have some symbols that are defined in later versions, which led to an error for some users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants