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

Negative Binary Literals are Positive #235

Closed
almann opened this issue Apr 25, 2021 · 1 comment
Closed

Negative Binary Literals are Positive #235

almann opened this issue Apr 25, 2021 · 1 comment

Comments

@almann
Copy link
Contributor

almann commented Apr 25, 2021

When implementing amazon-ion/ion-rust#214, I noticed in this commit that equiv testing was failing for binary notation of integers.

Specifically these two skip-list files:

    // ion-c seems to have a problem with negative binary literals amzn/ion-c#235
    "ion-tests/iontestdata/good/equivs/binaryInts.ion",
    "ion-tests/iontestdata/good/equivs/intsWithUnderscores.ion",

When running our Element tests in ion-rs, the values parse out as positive for the binary literals. I will add a direct reproduction in ion-c when I get a chance, but I wanted to make sure I cut the issue so we don't lose track of it.

@almann almann added the bug label Apr 25, 2021
almann added a commit to almann/ion-rust that referenced this issue Apr 25, 2021
* Adds `test-generator` create dev dependency to get test per file.
* Adds a test suite with `good`/`bad` files with skip lists.
  - `equivs` and `non-equivs` are also supported with
    `embedded_document` parsing.
* Adds `Loader::load_all` convenience method.
* Adds doc test example in `loader` module docs and fixes the iterator
  example.

Additional notes:
* Added amazon-ion#216 as a result of non-equivs testing and it definitely exposes
  a weakness in our unit tests and bug in our `PartialEq` for `...Struct`.
* Added amazon-ion/ion-c#235 as these tests run into problems with negative
  binary integer syntax (but not hex or decimal), indicating a potential
  issue there.

Resolves amazon-ion#214.
almann added a commit to almann/ion-rust that referenced this issue Apr 25, 2021
* Adds `test-generator` create dev dependency to get test per file.
* Adds a test suite with `good`/`bad` files with skip lists.
  - `equivs` and `non-equivs` are also supported with
    `embedded_document` parsing.
* Adds `Loader::load_all` convenience method.
* Adds doc test example in `loader` module docs and fixes the iterator
  example.

Additional notes:
* Added amazon-ion#216 as a result of non-equivs testing and it definitely exposes
  a weakness in our unit tests and bug in our `PartialEq` for `...Struct`.
* Added amazon-ion#218 to track a particular set ion-c integration issues around integer
  sizes and binary reader.
* Added amazon-ion#219 to track adding support for unknown text symbol tokens in `Loader`.
* Added amazon-ion#220 to track adding `IonEq`.
* Added amazon-ion/ion-c#235 as these tests run into problems with negative
  binary integer syntax (but not hex or decimal), indicating a potential
  issue there.

Resolves amazon-ion#214.
@almann
Copy link
Contributor Author

almann commented Apr 25, 2021

Verified in a unit test in IonC:

// reproduction for amzn/ion-c#235
TEST(IonTextInt, BinaryLiterals) {
    const char *ion_text = "-0b100";
    hREADER  reader;
    ION_TYPE type;
    int64_t value;

    ION_ASSERT_OK(ion_test_new_text_reader(ion_text, &reader));
    ION_ASSERT_OK(ion_reader_next(reader, &type));
    ASSERT_EQ(tid_INT, type);
    ION_ASSERT_OK(ion_reader_read_int64(reader, &value));
    ASSERT_EQ(-4, value);
}

Produces the following failure:

Failure
      Expected: -4
To be equal to: value
      Which is: 4

almann added a commit to amazon-ion/ion-rust that referenced this issue Apr 26, 2021
* Adds `test-generator` create dev dependency to get test per file.
* Adds a test suite with `good`/`bad` files with skip lists.
  - `equivs` and `non-equivs` are also supported with
    `embedded_document` parsing.
* Adds `Loader::load_all` convenience method.
* Adds doc test example in `loader` module docs and fixes the iterator
  example.

Additional notes:
* Added #216 as a result of non-equivs testing and it definitely exposes
  a weakness in our unit tests and bug in our `PartialEq` for `...Struct`.
* Added #218 to track a particular set ion-c integration issues around integer
  sizes and binary reader.
* Added #219 to track adding support for unknown text symbol tokens in `Loader`.
* Added #220 to track adding `IonEq`.
* Added amazon-ion/ion-c#235 as these tests run into problems with negative
  binary integer syntax (but not hex or decimal), indicating a potential
  issue there.
* Added frehberg/test-generator#12 to capture the empty glob issue.

Resolves #214.
cheqianh added a commit to cheqianh/ion-c that referenced this issue Apr 28, 2021
almann added a commit to almann/ion-rust that referenced this issue Apr 28, 2021
* Removes skip-list for amazon-ion/ion-c#235.
* Updates `ion-c` and `ion-tests` submodules to HEAD.
* Fixes URL in ion-c submodule.
almann added a commit to amazon-ion/ion-rust that referenced this issue Apr 28, 2021
* Removes skip-list for amazon-ion/ion-c#235.
* Updates `ion-c` and `ion-tests` submodules to HEAD.
* Fixes URL in ion-c submodule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants