Bitsets3

[wclodius2]

Branch to implement bitset data types. Committed the following new source files:
src/stdlib_bitsets.f90
src/stdlib_bitset_64.f90
src/stdlib_bitset_large.f90

Added the following new test files:
src/tests/bitsets/CMakeLists.txt
src/tests/bitsets/Makefile.manual
src/tests/bitsets/test_stdlib_bitset_64.f90
src/tests/bitsets/test_stdlib_bitset_large.f90

Added the following new documentation file:
doc/specs/stdlib_bitssets.md

Modified the following compilation files:
src/CMakeLists.txt
src/Makefile.manual
src/tests/CMakeLists.txt
src/tests/Makefile.manual

[jvdp1]

Thank you @wclodius2 for this PR. I will try to review it this weekend or next week.

[14NGiestas]

There's a misspelling of the word 'occurred' in this file: "occured" (line 1092)
EDIT: this file

[wclodius2]

Looks good.

…

On Oct 4, 2020, at 11:49 AM, Jeremie Vandenplas ***@***.***> wrote: @jvdp1 commented on this pull request. In src/tests/Makefile.manual <#239 (comment)>: > @@ -10,6 +11,7 @@ all: test: $(MAKE) -f Makefile.manual --directory=ascii test + $(MAKE) -f Makefile.manual --directoru=bitsets test ⬇️ Suggested change - $(MAKE) -f Makefile.manual --directoru=bitsets test + $(MAKE) -f Makefile.manual --directory=bitsets test This should allow the Github actions to be successful — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#239 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APTQDOWITSG6CMZ2MNAX2VLSJCYSBANCNFSM4R6QFU5A>.

[14NGiestas]

We should really discuss the error handling further in #219 . The 'go to' usage in this function... I'm not a fan xD. I think only one labeled part is needed to achieve the 'return with status' vs 'stop program with error message' desired behavior, with the aid of a error handling module, we could set the message and a flag to switch between the two possible actions, reusing the code. I will bring some working code snippet soon (I hope).
Also, in the assigment functions I wonder if it is possible to use the template system (as in the statistics module)?

[wclodius2]

What is xD? The error handling has several aspects that could be commented on:

1. Rather than intermingling the error handling with the main logic I jump to error handling at the end of the procedure. This increases the amount of code slightly, and can obscure what the error handling is doing, but I think makes the main logic clearer.

2. I am handling some "errors" that possibly shouldn't be handled: memory allocation problems and user errors. With virtual memory, its not clear to me that allocation can ever fail, and with lazy allocation on Linux such failure will not occur on the allocate statement.. While not important in the procedure you call out, I also attempt to provide a fallback for some user errors where simple failure might be best.

3. Since we lack an error handling procedure I am relying explicitly on an if branch with error stop. An error handling procedure could incorporate the if branch and slightly simplify the logic by incorporating both a message and a status argument. There are however a lot of issues to be dealt with in developing an error handling module.

   • Should it be a separate module or part of the logger?

   • Should it include a set of error codes?

   • What should the arguments be to the main error handling procedure?

     • message
     • module and procedure names
     • status/stat flag
     • error code

   • How should processing be terminated, an error stop: with an F2018 runtime determined stop code, with an error code determined compile time stop code, generic compile time stop code?

4. Including the module and procedure names in the error stop makes the stop code longer and harder to read

5. Making the stop code message a character string parameter would shorten the error handling code, at the expense of cluttering up other portions of the code.

It is of course possible to use fypp preprocessing to generate templates for the assignments to and from logical arrays. The assignment procedures are few enough and simple enough that I thought it best to avoid a dependence on the preprocessor, but that is a matter of taste. The addition of more assignments, say to and from integers would probably change my opinion.

[14NGiestas]

What is xD?

ASCII version of 😆smiling face with closed eyes

1-5. Yes, the current way increases the amount of code, however the main logic can be still clear if we do something like that:

    subroutine fail_behavior(..., status)
        ! doing a lot of stuff here
        ! ...
        ! case X: found some fatal error
            error_handler % status = ERROR_CODE_TO_X
            error_handler % message = "Oh no! X happened"
            go to 404
        ! case Y: found other general error
            error_handler % status = ERROR_CODE_TO_Y
            error_handler % message = "Oh no! Y happened"
            go to 404

    404 if (present(status)) then
            status = error_handler % status
            return
        else
            error stop error_handler % message
        end if
    end subroutine

This is a way we can avoid the code duplication, reusing the go to branching. Note that error_handler should be a object that couples the information about what happened (not reflecting in any way the API I got in mind for a final version).
The error handling should be a separate module with some default general purpose errors and a abstract one, that would allow it to be extended when needed by other modules, this way they can provide their own specific codes constants and messages.

It is of course possible to use fypp preprocessing to generate templates for the assignments to and from logical arrays. The assignment procedures are few enough and simple enough that I thought it best to avoid a dependence on the preprocessor, but that is a matter of taste. The addition of more assignments, say to and from integers would probably change my opinion.

IMHO, we should use the template system every time that not using it leads to code duplication, since it gets easier to modify and extend the code in the future. (DRY principle).

[wclodius2]

FWIW for the error handling what I thought you were proposing was something like

subroutine fail_behavior(..., status)
    ! doing a lot of stuff here
    ! ...
    ! case X: found some fatal error
        call error_handler(message="Oh no! X happened", error=error_code_to_x, status=status)
        return
    ! case Y: found other general error
        call error_handler(message="Oh no! Y happened", error=error_code_to_y, status=status)
        return
end subroutine

where error_handler assigns error to status if status is present and stops processing with a message to error_unit otherwise.

I can change the code to use fypp, but it probably requires starting a new branch. I can see how to add or modify files in the current branch, but not how to rename or remove existing files, which I need to change the *.f90 files to *.fypp.

[14NGiestas]

FWIW for the error handling what I thought you were proposing was something like (...)
where error_handler assigns error to status if status is present and stops processing with a message to error_unit otherwise.

Indeed, actually this is even better because we don't need retype the branching logic with "go to" statements in every function that may fail and is doable (working example here). To avoid having any boilerplate along the function's main logic, the module that have a specific need should build a function that calls this base function error_handler, Ex:

subroutine value_error(expected, got, status)
     ....
     write(message, '("value_error: Expected ",G0,", but got ",G0," instead.")') expected, got
     call error_handler(message, ...) 
end subroutine

I can change the code to use fypp, but it probably requires starting a new branch. I can see how to add or modify files in the current branch, but not how to rename or remove existing files, which I need to change the *.f90 files to *.fypp.

You can clone your repo here, modify it locally and push the commits (It will probably sync with the open PR)

[jvdp1]

I can see how to add or modify files in the current branch, but not how to rename or remove existing files, which I need to change the *.f90 files to *.fypp.

You can rename and remove a file locally with your OS, and then add/remove, commit, and push with git.

[wclodius2]

When I try

git clone https://github.com/wclodius2/stdlib/tree/bitsets3 my-bitsets

I get the error message

Cloning into 'my-bitsets'...
fatal: repository 'https://github.com/wclodius2/stdlib/tree/bitsets3/' not found

Is there something wrong with my syntax/repository name?

[14NGiestas]

This is the wrong URL, please do the following:

git clone https://github.com/wclodius2/stdlib.git my-bitsets
cd my-bitsets
git checkout bitsets3

[jvdp1]

Suggested change

	use, intrinsic :: iso_fortran_env, only : int8, int16, int32, int64
	use stdlib_kinds, only : int8, int16, int32, int64

[jvdp1]

Here are some additional comments on the tests. All options seem to be covered by the tests. I added some suggestions.
On overall I am pleased with this PR. Thank you @wclodius2!

[wclodius2]

It means I wrote the code when I was tired and wanted someone else to check my reasoning. It is parsing a decimal representation of the size in bits, and I want to be certain that the size does not exceed what can be represented by an integer of `bits_kind`. That could happen for example if parsing a file with `bits_kind==int32` and the file was written with `bits_kind==int64`. We want to be sure that the representation for `bits_kind==int32` does not exceed `2**31-1`. My reasoning is that it will exceed that value if: 1. it has ten digits and is about to read another digit increasing its value by a factor of ten 2. It has nine digits and has a value greater than `2**31/10` 3. It had a value of `2**31/10` and reading the next digit causes an integer overflow of nine or less resulting in a negative value.

…

On Oct 20, 2020, at 11:46 AM, Jeremie Vandenplas ***@***.***> wrote: @jvdp1 commented on this pull request. In src/stdlib_bitsets_large.fypp <#239 (comment)>: > + end do find_start + + if ( pos > len(string) - 8 ) go to 999 + + if ( string(pos:pos) /= 's' .AND. string(pos:pos) /= 'S' ) go to 999 + + pos = pos + 1 + bits = 0 + digits = 0 + + do + select case( iachar( string(pos:pos) ) ) + case(ia0:ia9) + digits = digits + 1 + if ( digits == max_digits .AND. bits > overflow_bits ) go to 996 +!! May not be quite right ⬇️ Suggested change -!! May not be quite right +!! May not be quite right What does this comment mean? What could be not quite right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#239 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APTQDOUBL3KX6BCX26LADBTSLXEG5ANCNFSM4R6QFU5A>.

[wclodius2]

I have now resolved my qualms about my handling of potential integer overflows on reads. I have changed max_digits for bits_kind==int64 from 20 to 19 (2**63-1 is about 9.2E18) and convinced myself that the rest of my logic was correct. I have move the comments that the code may not be quite right. (I also fixed a typo in an error message.)

[wclodius2]

@jvdp1 thanks for the thorough review.

[milancurcic]

I have reviewed and made edits to the spec document. I have also built the code run the tests (successfully). For as far as I understand bitsets, I think the API is good.

I did not review the code. Due to the combination of limited time, large size of this PR, and my unfamiliarity with bitsets, I don't think I can do proper code review without putting in substantial effort. But I trust @wclodius2 and @jvdp1 that they did great work.

I recommend this PR to be merged a week from now.

[wclodius2]

A bit set for each index `bit` in the range `0…bits-1` has a value of 0 or 1. A value of 1 for index `bit` can be considered as including the `bit` value in the subset, i.e., the value `1110` can be considered as defining the subset (1, 2, 3). This was how Wirth defined sets in Pascal.

…

On Nov 13, 2020, at 11:42 AM, Milan Curcic ***@***.***> wrote: @milancurcic commented on this pull request. In doc/specs/stdlib_bitsets.md <#239 (comment)>: > +equivalently be considered as a sequence of logical values or as a +subset of the integers 0 ... `bits-1`. The bits are indexed from 0 to or as a subset of the integers 0 ... bits-1. This confuses me. The equivalent integer values are 0 and 1, not 0 through bits-1, correct? Do you mean to say something like this? "It can equivalently be considered as a sequence of logical values or as a sequence of integers 0 and 1 with indices in the range 0... bits-1?" — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#239 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APTQDOTR4LOOIPRJ6D2QW63SPV42PANCNFSM4R6QFU5A>.

[milancurcic]

Okay, thanks, I understand now. All good.

[milancurcic]

Your example "i.e., the value 1110 can be considered as defining the subset (1, 2, 3)" was what made it click for me. Do you agree that we include it in the spec? Something like this:

"It can equivalently be considered as a sequence of logical values or as a subset of the integers 0 ... bits-1. For example, the value 1110 can be considered as defining the subset of integers [1, 2, 3]."

[wclodius2]

Sounds good to me.

[milancurcic]

I will go ahead and merge this considering there aren't any objections. Thank you @wclodius2 and all the reviewers.