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

R2_590 change ESIL pop signature #21887

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

luc-tielen
Copy link
Collaborator

@luc-tielen luc-tielen commented Jun 12, 2023

  • Mark this if you consider it ready to merge
  • I've added tests (optional)
  • I wrote some lines in the book (optional)

Description

Merge #21886 first, also this doesn't fix all warnings yet..

@luc-tielen luc-tielen changed the title Rm 590 pt2 R2_590 change ESIL pop signature Jun 12, 2023
@trufae
Copy link
Collaborator

trufae commented Jan 19, 2024

comments on this @condret ?

@trufae
Copy link
Collaborator

trufae commented Jul 16, 2024

Can you rebase this PR we should target r2-6.x now

@condret
Copy link
Member

condret commented Aug 8, 2024

sorry for the late response. i'm not a fan of this. I don't see any necessity, and all these casts to char * feel bloaty to me. We can do this, if you really want this

@trufae
Copy link
Collaborator

trufae commented Aug 8, 2024

Theres a noticeable perf boost by not strduping every word. Thats the reason behind this patch. So maybe the casts are ugly but i wanted to make clear the intentions behind

@condret
Copy link
Member

condret commented Aug 8, 2024

can't we then just rm the strdups

@trufae
Copy link
Collaborator

trufae commented Aug 8, 2024

Nope i tried. Some because invalid pointers. So we need again an abstraction over strings

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