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

RDS writer #16

Merged
merged 34 commits into from
Jul 18, 2024
Merged

RDS writer #16

merged 34 commits into from
Jul 18, 2024

Conversation

breitnw
Copy link
Collaborator

@breitnw breitnw commented Jul 3, 2024

No description provided.

programLyrique and others added 26 commits February 22, 2024 17:14
@programLyrique
RDSWriter
bdd280d
@programLyrique
Add information in the README:
50f12a6 
- requires java 21 at least
- requires to have npm installed
@programLyrique
RDSOutputStream
271b2a2
@programLyrique
Minimal compiling RDSWriter
5d4135c
@programLyrique
Names of symbol should be non null.
0abd2f0
@programLyrique
Support most common sexp types for RDS writing
a99082a
@programLyrique
Write read test for integer vector
a5e6af9
@programLyrique
Test for Logical + associated builder
eb4174c
@programLyrique
Tests for reals and null
c634080
@programLyrique
Tests writing with GNU R as oracle
a133a1c
@programLyrique
ListSXP works.
f6e6171 
Type should be written as int not byte!
@programLyrique
Supports user environments
6a946a6
@programLyrique
Bytecode serialization: WIP
f4c144e
@programLyrique
Some stubs I was starting to fill for the bytecode writer
da47ea4
@breitnw
Merge branch 'main' of github.com:PRL-PRG/r-compile-server into rds-w…
d95424b 
…riter

# Conflicts:
#	.settings/org.eclipse.jdt.core.prefs
#	src/main/java/org/prlprg/bc/BcCode.java
#	src/main/java/org/prlprg/sexp/ListSXP.java
#	src/main/java/org/prlprg/sexp/NamespaceEnvSXP.java
#	src/main/java/org/prlprg/sexp/RegSymSXP.java
#	src/main/java/org/prlprg/sexp/SEXP.java
#	src/main/java/org/prlprg/sexp/UserEnvSXP.java
#	src/test/java/org/prlprg/util/GNUR.java
@breitnw
Support for complex number serialization
71bce82
@breitnw
Writer and RDS flag refactor, basic GP flag support
c03929c
@breitnw
Bytecode serialization should probably work (untested)
11f2226
@breitnw
Bytecode serialization actually works (tested)
f80b079
@breitnw
Bytecode serialization actually works (tested)
b0aed80
@breitnw
Merge branch 'rds-writer' of github.com:PRL-PRG/r-compile-server into…
13f1f57 
… rds-writer
@breitnw
Merge branch 'main' of github.com:PRL-PRG/r-compile-server into rds-w…
6a12ca8 
…riter

# Conflicts:
#	src/main/java/org/prlprg/sexp/BaseEnvSXP.java
#	src/main/java/org/prlprg/sexp/EmptyEnvSXP.java
#	src/main/java/org/prlprg/sexp/EnvSXP.java
#	src/main/java/org/prlprg/sexp/GlobalEnvSXP.java
#	src/main/java/org/prlprg/sexp/ListSXP.java
#	src/main/java/org/prlprg/sexp/RegSymSXP.java
#	src/main/java/org/prlprg/sexp/SEXP.java
#	src/main/java/org/prlprg/sexp/UserEnvSXP.java
@breitnw
Compiler tests passing
2ec7ca2
@breitnw
Reference serialization
a54dace
@breitnw
Removed obsolete TODOs
ab9f412
@breitnw
fixed verification (hopefully)
dff1661
programLyrique
programLyrique reviewed Jul 4, 2024
View reviewed changes
src/main/java/org/prlprg/rds/GNURByteCodeEncoderFactory.java Show resolved Hide resolved
src/main/java/org/prlprg/rds/GNURByteCodeEncoderFactory.java Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSReader.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSWriter.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSWriter.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSWriter.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSWriter.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSWriter.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSWriter.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/sexp/GlobalEnvSXP.java Outdated Show resolved Hide resolved
@breitnw breitnw marked this pull request as draft July 11, 2024 13:40
programLyrique
programLyrique reviewed Jul 11, 2024
View reviewed changes
Copy link
Collaborator

@programLyrique programLyrique left a comment

Choose a reason for hiding this comment

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

I feel like the design of adding desc to write/read methods create too much coupling between the writer and the logger.

src/main/java/org/prlprg/rds/RDSInputStream.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSLogger.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSLogger.java Outdated Show resolved Hide resolved
@breitnw
RDS roundtrip test with R closures
4a7a285 
Several tests are not yet passing because character encodings are not fully implemented
@programLyrique programLyrique requested a review from fikovnik July 16, 2024 09:13
@breitnw
Copy link
Collaborator Author

breitnw commented Jul 16, 2024

Note that not all of the tests are passing yet, since there are still issues with encoding for characters more than 1 byte. I'll be fixing those today, and also working on the logger to decouple it from the input and output streams (or removing it entirely)

@breitnw breitnw marked this pull request as ready for review July 17, 2024 17:21
fikovnik
fikovnik approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member

@fikovnik fikovnik left a comment

Choose a reason for hiding this comment

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

Great!

Two more things:

  • does this PR need to change the .idea / LSP files?
  • there are some IDEA warnings that you might want to address

README.md Outdated Show resolved Hide resolved
src/main/java/org/prlprg/bc/ConstPool.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSInputStream.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSReader.java Outdated Show resolved Hide resolved
src/main/java/org/prlprg/rds/RDSReader.java Outdated Show resolved Hide resolved
src/test/java/org/prlprg/rds/RDSRoundtripTest.java Outdated Show resolved Hide resolved
src/test/java/org/prlprg/rds/RDSRoundtripTest.java Outdated Show resolved Hide resolved
src/test/java/org/prlprg/rds/RDSWriterTest.java Outdated Show resolved Hide resolved
src/test/java/org/prlprg/rds/RDSWriterTest.java Outdated Show resolved Hide resolved
src/test/java/org/prlprg/rds/RDSWriterTest.java Outdated Show resolved Hide resolved
@breitnw breitnw merged commit 25912fe into main Jul 18, 2024
4 checks passed
@breitnw breitnw deleted the rds-writer branch July 18, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@programLyrique programLyrique programLyrique left review comments

@fikovnik fikovnik fikovnik approved these changes

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
@breitnw @fikovnik @programLyrique