Skip to content

Few types change can be safely #1624

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

Open
wants to merge 2 commits into
base: v2/master
Choose a base branch
from
Open

Few types change can be safely #1624

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 28, 2017

used afterwards (for apr_sleep for example it is an uint64_t-ish type as
parameter).

@zimmerle zimmerle added the 2.x Related to ModSecurity version 2.x label Feb 28, 2018
@victorhora victorhora added this to the v2.9.3 milestone Nov 10, 2018
@victorhora victorhora self-assigned this Nov 10, 2018
@victorhora victorhora self-requested a review November 11, 2018 18:42
@victorhora
Copy link
Contributor

Hi @dcarlier-afilias

Thanks for your contribution!

Did you got compiler warnings about these ones? :)

I'm also thoughtful about changing the type of rule_id. I'm thinking there would be other places in the code where rule_id is used and would also need to change the type from int to long.

@ghost
Copy link
Author

ghost commented Nov 13, 2018

I did get compiler warnings indeed for those lines.

David Carlier added 2 commits November 18, 2018 17:29
used afterwards (for apr_sleep for example it is an uint64_t-ish type as
parameter).
@ghost
Copy link
Author

ghost commented Nov 21, 2018

@victorhora What do you think of these now ?

@marcstern
Copy link

Wouldn't it be better to always use long instead of (sometimes) unsigned long?
Especially as there are some tests <= 0.
Otherwise looks good, except for build/compile where change don't look related to this PR, right?

@airween
Copy link
Member

airween commented May 16, 2024

@marcstern:

I see this is a pretty old PR, and then there wasn't any CI workflow. If you think this is a useful modification, could you sent this PR again in a new PR? Of course we can mention the original author and give the credit in CHANGELOG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants