Skip to content

Add File internals that work directly with Path#2869

Merged
harendra-kumar merged 9 commits intomasterfrom
file-open-direct
Apr 30, 2025
Merged

Add File internals that work directly with Path#2869
harendra-kumar merged 9 commits intomasterfrom
file-open-direct

Conversation

@adithyaov
Copy link
Copy Markdown
Member

@adithyaov adithyaov commented Oct 15, 2024

No description provided.

@adithyaov adithyaov force-pushed the file-open-direct branch 4 times, most recently from 9ffc75b to da8093e Compare October 29, 2024 12:07
@adithyaov adithyaov linked an issue Oct 30, 2024 that may be closed by this pull request
Use the Path type in the File module #2770
Closed
@adithyaov
Copy link
Copy Markdown
Member Author

adithyaov commented Nov 4, 2024

Ensure the change made is tested in the CIs

@adithyaov adithyaov force-pushed the file-open-direct branch 2 times, most recently from a48ea71 to c91d3f6 Compare November 4, 2024 11:32
harendra-kumar
harendra-kumar requested changes Nov 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@harendra-kumar harendra-kumar left a comment

Choose a reason for hiding this comment

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

Change the module names as follows:

FileSystem.Windows.File => FileSystem.File.Window
FileSystem.Posix.File => FileSystem.File.Posix

@harendra-kumar harendra-kumar force-pushed the file-open-direct branch 3 times, most recently from 73fc760 to c348768 Compare April 14, 2025 08:25
@harendra-kumar harendra-kumar mentioned this pull request Apr 15, 2025
Merged
@harendra-kumar harendra-kumar force-pushed the file-open-direct branch 2 times, most recently from 06e0c7e to 9771953 Compare April 22, 2025 14:39
adithyaov
adithyaov commented Apr 23, 2025
View reviewed changes
Comment thread core/src/Streamly/Internal/FileSystem/FileIO.hs
, writeAppendArray
, writeAppendChunks

-- * Deprecated
Copy link
Copy Markdown
Member Author

@adithyaov adithyaov Apr 23, 2025

Choose a reason for hiding this comment

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

You can remove this section, as this is a new module.

Copy link
Copy Markdown
Member

@harendra-kumar harendra-kumar Apr 24, 2025

Choose a reason for hiding this comment

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

I though it will be easier to replace the module if we keep them. But we can remove it too, because if someone is anyway making changes then they can change this as well.

adithyaov
adithyaov commented Apr 23, 2025
View reviewed changes
Comment thread core/src/Streamly/Internal/FileSystem/Posix/File.hsc
name2 :: FileMode -> FileMode; \
name2 (FileMode mode) = FileMode (x .|. mode)

{-
Copy link
Copy Markdown
Member Author

@adithyaov adithyaov Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why did you comment this out?
So you can avoid using ##?

Copy link
Copy Markdown
Member

@harendra-kumar harendra-kumar Apr 24, 2025

Choose a reason for hiding this comment

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

No, we are using the definitions directly from the header files rather than defining ourselves. I kept this code commented, just in case there is any issue in using the headers directly.

Copy link
Copy Markdown
Member Author

@adithyaov adithyaov Apr 24, 2025

Choose a reason for hiding this comment

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

Does commenting out CPP definitions even work?

Copy link
Copy Markdown
Member Author

@adithyaov adithyaov Apr 24, 2025

Choose a reason for hiding this comment

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

But if it did not error out because you've not used ## it likely works as expected.

Copy link
Copy Markdown
Member

@harendra-kumar harendra-kumar Apr 27, 2025

Choose a reason for hiding this comment

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

This code is actually commented, it does not work in any way.

adithyaov
adithyaov commented Apr 23, 2025
View reviewed changes
Comment thread core/src/Streamly/Internal/FileSystem/Posix/File.hsc
adithyaov
adithyaov commented Apr 23, 2025
View reviewed changes
Comment thread core/src/Streamly/Internal/FileSystem/Posix/File.hsc Outdated
Comment on lines +112 to +116
name1 :: FileMode -> FileMode; \
name1 (FileMode mode) = FileMode (x .|. mode); \
{-# INLINE name2 #-}; \
name2 :: FileMode -> FileMode; \
name2 (FileMode mode) = FileMode (x .|. mode)
Copy link
Copy Markdown
Member Author

@adithyaov adithyaov Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why do name1 and name2 have the same implementations?
set and clear have the same implementation?
Should name2 be something like x .&. complement mode instead?

Copy link
Copy Markdown
Member

@harendra-kumar harendra-kumar Apr 24, 2025

Choose a reason for hiding this comment

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

Yes, that's a bug. copy, paste and forgot to fix.

adithyaov
adithyaov commented Apr 23, 2025
View reviewed changes
Copy link
Copy Markdown
Member Author

@adithyaov adithyaov left a comment

Choose a reason for hiding this comment

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

I've reviewed the PR. I've added some comments.

adithyaov and others added 7 commits April 30, 2025 11:05
@adithyaov @harendra-kumar
Add File internals that work directly with Path
06ba517
@adithyaov @harendra-kumar
Use the new File functions with Path in the Handle testsuite
ec69357
@harendra-kumar
Fix deprecated APIs in FileIO
7b8bf7d
@harendra-kumar
Move Windows/File.hs to File.hsc
b60c43f
@harendra-kumar
Fix a bug, cleanup the Posix,Windows File modules
944b4c7 
Use asCWStringUnsafe to create a null terminated C string in windows
module.

Export openFile functions directly from the platform specific module and
reverse the dependency on Utils module.
@harendra-kumar
Move Posix/File.hs to Posix/File.hsc
210cde6
@harendra-kumar
Change openAt API
0d571c0 
Now we can set and clear open flags and create mode using function based API.
@harendra-kumar harendra-kumar merged commit c3cb522 into master Apr 30, 2025
16 of 21 checks passed
@harendra-kumar harendra-kumar deleted the file-open-direct branch May 3, 2025 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@harendra-kumar harendra-kumar harendra-kumar requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use the Path type in the File module

2 participants

@adithyaov @harendra-kumar