Skip to content

c_long -> int64 for content_length #12

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

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

milancurcic
Copy link
Member

#7 introduced a missing import which broke the build (a case for why we need tests #9 and CI next).

While adding it, I realized that content_length should be c_size_t instead of c_long. Probably no difference in practice, both seem to work, but I naively expect c_size_t to be more correct since content length is unsigned.

@rajkumardongre
Copy link
Contributor

rajkumardongre commented May 31, 2023

Thanks @milancurcic, I have only one concern regarding this matter. It appears that the size of c_size_t is dependent on the architecture and operating system, which means it can either be 32 bits (4 bytes) or 64 bits (8 bytes). Additionally, the content_length attribute may exceed the maximum size of a 4-byte integer on a 32-bit platform. To address this issue, it is recommended to declare the content_length attribute as an 8-byte integer, which can accommodate larger values than a 4-byte integer.

Although 8-byte integers are typically used on 64-bit platforms, they are also available on 32-bit platforms in Fortran. Therefore, declaring the content_length attribute as an 8-byte integer ensures that it can be represented accurately on both 32-bit and 64-bit platforms.

By using an 8-byte integer for content_length, the attribute can store larger values, which may be necessary for handling large HTTP messages. This approach ensures that the content_length attribute is accurately represented and can be used reliably in a Fortran program, regardless of the platform it is running on.

@interkosmos has also commented on this matter in their remarks.

Just a few remarks:

  • The status_code in response_type has no default value. HTTP status codes should be added as public parameters. The attribute content_length should be an 8-byte integer, as it is available in Fortran on 32-bit platforms too.

@milancurcic
Copy link
Member Author

Thanks for that explanation, and indeed I missed that comment by @interkosmos. In that case, we can keep content_length as c_long, and this PR only needs to make c_long accessible from iso_c_binding, do you agree?

@rajkumardongre
Copy link
Contributor

I think c_long is also platform, OS, and compiler-dependent, which means that its value can be either 4 bytes or 8 bytes. Therefore, the problem will remain the same. That's why I believe that using an 8-byte integer, as suggested by @interkosmos, would be a better solution. For example, can we use this syntax to declare content_length? :

integer(kind=8), public :: content_length = 0

This syntax ensures that the variable is always 8 bytes in size, regardless of the platform and compiler being used, and avoids potential issues that could arise from using c_long.

I'm aware that using kind=8 specifies a hardcore size for the variable, but I can't think of another way to ensure that the variable is always 8 bytes in size. If there is a better way to achieve this, I'd be interested to learn about it.

@interkosmos
Copy link
Member

I would prefer int64 from iso_fortran_env because c_size_t leads to problems when trying to write platform-independent code. The library should hide any type conversions and offer a consistent 8-byte content_size. As fas as I know, kind=8 is not available in all Fortran compilers (thus, using the kind from iso_fortran_env).

@milancurcic
Copy link
Member Author

Thanks, that makes sense, we'll use int64 then.

@milancurcic milancurcic merged commit 2bee99b into fortran-lang:main Jun 1, 2023
@milancurcic milancurcic changed the title c_long -> c_size_t for content_length c_long -> int64 for content_length Jun 1, 2023
@milancurcic milancurcic deleted the fix-missing-import branch June 1, 2023 13:22
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