Skip to content

implemented const_cast as a macro#7045

Open
badasahog wants to merge 3 commits intomasterfrom
constCastMacro
Open

implemented const_cast as a macro#7045
badasahog wants to merge 3 commits intomasterfrom
constCastMacro

Conversation

@badasahog
Copy link
Contributor

I changed const_cast to be a type agnostic macro, much more in line with the C++ version.

@badasahog badasahog requested a review from aitap as a code owner June 4, 2025 12:39
@github-actions
Copy link

github-actions bot commented Jun 4, 2025

  • HEAD=constCastMacro slower P<0.001 for memrecycle regression fixed in #5463
  • HEAD=constCastMacro slower P<0.001 for setDT improved in #5427
    Comparison Plot

Generated via commit 726b126

Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 5 minutes and 7 seconds
Installing different package versions 10 minutes and 32 seconds
Running and plotting the test cases 2 minutes and 38 seconds

@aitap
Copy link
Member

aitap commented Jun 24, 2025

This seems to crash fread. We only have one use for casting away the const: to fiddle with the memory-mapped file. There is no need for the generic version. If you would like to get rid of union type punning, let's launder the non-const pointer through uintptr_t. If you have time, try to find out which compiler emits warnings when casting a const char * to char * - what if we don't have to worry about it?

union { const char *a; char *b; } tmp = { ptr };
return tmp.b;
}
#define const_cast(x) (((union{typeof(x) a; typeof(+(*(x)))* b;}){.a=(x)}).b)
Copy link
Member

Choose a reason for hiding this comment

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

In the issue titled 'awful one liners don't improve readability or performance' (#6995) I see you wrote:

Collapsing everything into one line doesn't make the code run faster, the compiler is smart enough to sort it out.

I wonder what category this belongs to then 🤔

Copy link
Contributor Author

@badasahog badasahog Jun 29, 2025

Choose a reason for hiding this comment

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

A fair criticism!

The problem is, I don't think it's even possible to implement it differently since its a macro, and I think const_cast already has a clear enough meaning that the one liner isn't too bad here. It's clear what the code does.

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

Successfully merging this pull request may close these issues.

3 participants