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.

Sign up for GitHub

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

Jump to bottom

fixes #23442, fix for FileId under Windows #23444

Merged
merged 10 commits into from
May 8, 2024
Merged

fixes #23442, fix for FileId under Windows #23444

merged 10 commits into from
May 8, 2024

Conversation

litlighilit
Copy link
Contributor

@litlighilit litlighilit commented Mar 27, 2024 •
edited
Loading

See according issue:

Details:
#23442 (comment)

litlighilit and others added 8 commits February 21, 2024 16:50
@litlighilit
Update browsers.nim. remove unimplemented openDefaultBrowser()
881973c 
`openDefaultBrowser()` failed to match its desired functionality

What's more, meaningless and unworthy to fix it
@litlighilit
Update browsers.nim, several renaming of inner procedures
3a7cde1 
rename `openDefaultBrowserImplPrep` to `openDefaultBrowserRaw`

and remove unnecessary `openDefaultBrowserImpl`
@litlighilit
Update browsers.nim, deprecate openDefaultBrowser() instead of remo…
c71ca8c 
…ving

and rewrite its document accordingly.
@litlighilit @ringabout
Update lib/pure/browsers.nim, fix a typo
ae03cda 
"specifical" is a non-standard variation of "specific"

Co-authored-by: ringabout <43030857+ringabout@users.noreply.github.com>
@litlighilit
Update browsers.nim, add forgetten since module
a2f20fe 
add `import std/private/since`
@litlighilit
Update browsers.nim, put back since import to the head of file
fa77ff5
Merge branch 'nim-lang:devel' into devel
8853bfe
@litlighilit
fixes nim-lang#23442, use uint64 for FileId under Windows over int64
2a94963
@rockcavera
Copy link
Contributor

A solution to maintain compatibility is to use cast[int64](), in the merge() template.

@litlighilit
Copy link
Contributor Author

litlighilit commented Mar 27, 2024 •
edited
Loading

A solution to maintain compatibility is to use cast[int64](), in the merge() template.

Then, ... for cases like #23442,

user will just get a overflowed negative integer

@rockcavera
Copy link
Contributor

user will just get a overflowed negative integer

Unfortunately, to avoid possible breakages, it's better to keep int64 and use cast. But it's just my opinion.

@litlighilit
Copy link
Contributor Author

According to my research described in #23442 (comment),

the type wrapped is unsigned, either in Windows or POSIX. (and this fix only affect windows)

Thus, there won't be any breakage, I think.

@Araq
Copy link
Member

Araq commented Mar 27, 2024

It's a breaking change, keep the type and use cast[int64]

@litlighilit
Copy link
Contributor Author

... then this issue is not handled, it just slience the overflow error, resulting in a negative value.

What if another issue comes out, saying FileId gives wrong negative integer?

@litlighilit
Copy link
Contributor Author

litlighilit commented Mar 27, 2024 •
edited
Loading

Publically, this fix does not affect public API;

Innerly, this fix just strips some unnecessary overflow check1
,

as FileId is merged by two DWORD, which in fact is unsigned.

When it comes to DWORD, one long-standing issue again comes out: DWORD is declared in winlean as int32 (a.k.a signed) (relative issue #12327)

Footnotes

  1. as int* is overflow-checked while uint* isn't ↩

@Araq
Copy link
Member

Araq commented Mar 27, 2024

What if another issue comes out, saying FileId gives wrong negative integer?

How could this happen? It's just a 64 bit number either way.

@litlighilit
Copy link
Contributor Author

litlighilit commented Mar 27, 2024 •
edited
Loading

What if another issue comes out, saying FileId gives wrong negative integer?

How could this happen? It's just a 64 bit number either way.

In the original issue #23442, this would happen, where a FileId is greater than high int64,

if just using cast[int64] in template merge, issue #23442 will be converted from raising RangeDefect to getting a negative value

@litlighilit
Copy link
Contributor Author

That's something like...
Do not solve the bug, but silence it

@rockcavera
Copy link
Contributor

That's something like... Do not solve the bug, but silence it

In fact, using cast[int64] solves the problem, as the bit arrangement does not change anything. Switching to uint64 will be a breaking change.

If the intention is to correct the WINAPI definitions (which have a lot of divergences), I believe that a single breaking change is better than making several breaking changes over time.

@litlighilit
Copy link
Contributor Author

litlighilit commented Mar 27, 2024 •
edited
Loading

Yeah, I do know the cast doesn't touch memory reperesentation...

So, we just have to reply to the original issue: "do not feel confused if you got a negative value. You can cast it back to uint64 to get its real representation"?

@rockcavera
Copy link
Contributor

Yeah, I do know the cast doesn't touch memory reperesentation...

So, we just have to reply to the original issue: "do not feel confused if you got a negative value. You can cast it back to uint64 to get its real representation"?

no need for any explanation, since int64 has always been the high-level implementation of Nim. If the person wants to get a uint64, he can use cast or use the low-level implementation (winlean) and merge the two uint32 provided by winapi into one uint64.

litlighilit added a commit to litlighilit/Nim that referenced this pull request Mar 27, 2024
@litlighilit
fixes nim-lang#23442, workaround RangeDefect via cast
9696e75 
See <nim-lang#23444 (comment)>
@litlighilit litlighilit mentioned this pull request Mar 27, 2024
Closed
Araq
Araq reviewed Mar 28, 2024
View reviewed changes
lib/pure/os.nim Outdated Show resolved Hide resolved
Araq
Araq reviewed Mar 28, 2024
View reviewed changes
lib/pure/os.nim Outdated Show resolved Hide resolved
@litlighilit litlighilit changed the title fixes #23442, use uint64 for FileId under Windows over int64 fixes #23442, fix for FileId under Windows May 5, 2024
@litlighilit
Copy link
Contributor Author

ping @Araq ,
is this okay?

@Araq Araq merged commit 6cc783f into nim-lang:devel May 8, 2024
19 checks passed
@Araq
Copy link
Member

Araq commented May 8, 2024

Sorry for the delay, pining me was the right thing to do.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 8, 2024

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 6cc783f

Hint: mm: orc; opt: speed; options: -d:release
178567 lines; 8.482s; 752.816MiB peakmem

narimiran pushed a commit that referenced this pull request May 23, 2024
@litlighilit @narimiran
fixes #23442, fix for FileId under Windows (#23444)
cd72be2 
See according issue:

Details:
<#23442 (comment)>

---------

Co-authored-by: ringabout <43030857+ringabout@users.noreply.github.com>
Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
(cherry picked from commit 6cc783f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@litlighilit @rockcavera @Araq