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

Fix issue #46 #48

Merged
merged 17 commits into from
Jul 1, 2020
Merged

Fix issue #46 #48

merged 17 commits into from
Jul 1, 2020

Conversation

JaniruTEC
Copy link
Member

@JaniruTEC JaniruTEC commented Jun 18, 2020

Added MAPPING.md as common guideline for type-conversions
Added new Annotation (Unsigned) used to indicate that an integer type or integer type pointer refers to an unsigned value.
Corrected types of fields in structures to match Dokany
Added Unsigned-Annotation to all accessors of unsigned values
Added UnsignedNumbers class with conversion methods between unsigned integer types and String

Merging this PR closes issue #46

Added new Annotation (Unsigned) used to indicate that an integer type or integer type pointer refers to an unsigned value.
Used Unsigned-Annotation to mark parameter length in getDokanControlList method in DokanControl and all calls that lead to it
Changed getDokanControlList to use an unsigned int instead of a long (matching Dokany that uses ULONG) and finished method (removed TODO)
Changed length of array MountPoint to 260 to match Dokany
Updated DokanControl to use WinNT.MAX_PATH and rewrote getDokanControlList
Corrected types of fields in structures to match Dokany
FIxed wrong types/usage in Constructor in DokanControl
FIxed wrong parameters in Constructor in DokanOptions
Fixed wrong parameters and added Unsigned-Annotation in accessors of the constructer
Added Unsigned-Annotation to all accessors of unsigned value
Added UnsignedNumbers class with conversion methods between unsigned integer types and String
Fixed toString method of DokanFileInfo and DokanOptions
@JaniruTEC JaniruTEC requested a review from infeo June 21, 2020 22:24
@JaniruTEC JaniruTEC linked an issue Jun 21, 2020 that may be closed by this pull request
Copy link
Member

@infeo infeo left a comment

Choose a reason for hiding this comment

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

Generally I approve, but the (already previously) discussed changes need to be implemented.

Moved my comment and took out the sass to make it more professional

See: dokan-dev#48 (comment)
Replaced usage of "assert" with explicit check to make sure that the check is done, even if assertions are disabled.

From the java language guide (linked below):
"Do not use assertions for argument checking in public methods.
Argument checking is typically part of the published specifications (or contract) of a method, and these specifications must be obeyed whether assertions are enabled or disabled."

The next sentence "Another problem with using assertions for argument checking is that erroneous arguments should result in an appropriate runtime exception (such as IllegalArgumentException, IndexOutOfBoundsException, or NullPointerException). An assertion failure will not throw an appropriate exception." doesn't apply in this case as this method isn't part of the API. It's only used internally and this check is implemented solely for the purpose of making sure everything in this library is working fine.

See: https://docs.oracle.com/javase/7/docs/technotes/guides/language/assert.html or https://web.archive.org/web/20200622193110/https://docs.oracle.com/javase/7/docs/technotes/guides/language/assert.html
@JaniruTEC
Copy link
Member Author

To document the reasoning for creating MAPPING.md:

  1. It's important to use the same mapping all over the project, therefore establishing a guideline seemed to be important.
  2. It gives easy access to any sources regarding this matter.
  3. It helps users of this API, as well as new developers (as well as old ones after a few weeks have passed) to understand the inner workings of the conversions between Dokany and dokan-java.

Added methods to compare, divide and get the remainder of unsigned numbers to UnsignedNumbers.

See: dokan-dev#48 (comment)
Added methods for unsigned conversion between integer types
Added constants that were missing from my last commit to UnsignedNumbers to allow compilation/fix compiler errors. Sorry!
Removed all unnecessary values from the @ Targets of @ Unsigned
infeo
infeo previously requested changes Jun 28, 2020
Copy link
Member

@infeo infeo left a comment

Choose a reason for hiding this comment

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

Add the Javadoc for the annotation and this PR is ready to be merged.

Comment on lines +79 to +80
* At least the following cases ("minimum usage") should be annotated with <i>@Unsigned</i> to
* guarantee the best quality of code:
Copy link
Member Author

Choose a reason for hiding this comment

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

In the future it may be useful to check our/foreign source code for compliance to this rule, e.g. with the Checker Framework.

Copy link
Member Author

Choose a reason for hiding this comment

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

// Do not resolve or close. //

@JaniruTEC JaniruTEC dismissed infeo’s stale review July 1, 2020 23:10

All changes implemented.

@JaniruTEC JaniruTEC merged commit 26b96fe into dokan-dev:develop Jul 1, 2020
@JaniruTEC JaniruTEC deleted the feature/fix-46 branch July 1, 2020 23:11
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.

Mismatching DokanOptions
2 participants