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

Deprecate <~> operator #19985

Closed
wants to merge 23 commits into from
Closed

Deprecate <~> operator #19985

wants to merge 23 commits into from

Conversation

benharsh
Copy link
Member

@benharsh benharsh commented Jun 13, 2022 •
edited
Loading

DRAFT

This PR updates our modules and tests to no longer use the <~> operator, and adds relevant deprecation warnings.

Testing:

  • full local
  • gasnet

TODO:

  • investigate weird ".read()" behavior in recordio.chpl
  • get feedback on stylistic changes
  • add deprecation test
  • update existing tests
  • Add compiler deprecation warning for user-defined <~>

@benharsh benharsh marked this pull request as draft June 13, 2022 21:22
@benharsh benharsh force-pushed the deprecate-readwrite-operator branch from 00dc68f to 9bdc8d1 Compare June 13, 2022 22:47
@benharsh
Copy link
Member Author

Note that this will depend on #19983 being merged.

@benharsh benharsh force-pushed the deprecate-readwrite-operator branch from 9bdc8d1 to c3959cb Compare June 15, 2022 03:43
benharsh added 21 commits June 15, 2022 14:19
@benharsh
Replace <~> in distributions with calls to write/writeln
2efe74b 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update package modules to no longer use <~>
da0184c 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update locale models to no longer use <~>
7c2ff31 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Trivial changes to no longer use <~> in internal modules
58366d5 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update DefaultAssociative to no longer use <~>
a3901fb 
Splits up dsiSerialReadWrite on DefaultAssociativeDom, and uses a nested
helper function for DefaultAssociativeArr's read/write methods.

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Add test to specifically exercise IO behavior when reading in Default…
b70dce7 
…Rectangular arrays using

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update DefaultRectangular to no longer use <~>
ac8a236 
Updates domain's dsiSerialReadWrite to use conditionals.

Updates other methods to use a nested helper function.

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update LayoutCS to no longer use <~>
dc58fd6 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Trivial updates to standard modules to no longer use <~>
a84e035 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update IO module to no longer use <~>
abdc772 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update DateTime module to no longer use <~>
6b899fe 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update Map to no longer use <~>
69efddc 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update ChapelIO to no longer use <~>
315a994 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Deprecate <~> operator
411ba49 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Add documentation note that <~> is deprecated
d7ca3a8 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Trivially update tests to no longer use <~>
42de93c 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update operator tests with deprecation warnings for <~>
e8d8eb8 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update record IO tests to no longer use <~>
3b08562 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update line number in .good file
72095d4 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Add compiler warning for user-defined <~> operators, and update tests
e11a320 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Add deprecation test for <~> operator
9affc91 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh benharsh force-pushed the deprecate-readwrite-operator branch from c3959cb to 9affc91 Compare June 15, 2022 22:55
@benharsh benharsh marked this pull request as ready for review June 15, 2022 23:08
@benharsh
Copy link
Member Author

@lydia-duncan : this is finally ready for review.

I've tried to make things look nice when possible, but there are a lot of if f.writing then f.write(....); else f.read(....); instances to replace <~>. We should consider whether this is acceptable.

In order to preserve the current behavior on main, I've used 'readIt' in our internal/standard/package modules rather than 'read'. This is because 'readIt' will throw an EEOF error, whereas 'read' will simply return false.

This bool-returning functionality is nice in a while loop, but isn't all that helpful when trying to implement 'readThis' on a type. Checking that returned value is a bit cumbersome in that case, hence the admittedly ugly changes in this PR to use readIt.

@benharsh
Copy link
Member Author

Anyways, I imagine this ultimately ties into some broader IO topic about the various read methods returning a bool. It would be nice if we could have something like swift here. Might look like while ! try? f.read(....) { }

@benharsh
fix test output with <~> deprecation warnings
ba089ad 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Fix prediff to remove only logging text
0ef5dad 
This test logs all array accessor calls, which could happen in any
number of places in the internal modules. If those array accessor calls
are shuffled around with IO changes, then the output of this test could
change unexpectedly and in a confusing way (e.g. an extra space
appearing, or disappearing).

This prediff targets and removes the exact array logging text, leaving
the other output untouched.

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Copy link
Member Author

@e-kayrakli : can you take a look at the prediff change to a local accessor optimization test (0ef5dad), and tell me whether this is reasonable or not?

@benharsh benharsh marked this pull request as draft June 16, 2022 18:07
e-kayrakli
e-kayrakli reviewed Jun 17, 2022
View reviewed changes
test/optimizations/autoLocalAccess/localArrays-1D-COO.good
@@ -12,7 +12,7 @@ End analyzing forall (localArrays.chpl:56)

Static check successful. Using localAccess (localArrays.chpl:57)
Testing 1D COO
2
1 2
Copy link
Contributor

Choose a reason for hiding this comment

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

While the changes look good, I can't understand the good file before your change. It looks like it should have always been printing out 1 2. Do you have a guess why it wasn't? I don't really remember.

Copy link
Member Author

@benharsh benharsh Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, the line printed was """1default _array accessor was called\n""" because the logging call was printed immediately before the write call for '2'. Previously the prediff was removing entire lines with the logging output, and also removed the '1'.

@benharsh benharsh mentioned this pull request Jul 7, 2022
Open
@benharsh
Copy link
Member Author

benharsh commented Aug 8, 2022

I'm going to split this into multiple PRs as the changes replacing the IO operator are probably worth reviewing independently of deprecation.

@benharsh benharsh closed this Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e-kayrakli e-kayrakli e-kayrakli left review comments

Assignees
No one assigned
Labels
post-release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benharsh @e-kayrakli