Add support for appending to a local symbol table, as prescribed by the spec.

E.g.

$ion_1_0
$ion_symbol_table::
{
  imports:[{ name:"com.amazon.ols.symbols.offer", version:1, max_id:75 },
           { name:"com.amazon.ims3.symbols.submission", version:1, max_id:100 }],
  symbols:["local_symbol"]
}
submission::{ /* ... */ local_symbol  /* ... */ }
$ion_symbol_table::
{
  imports:$ion_symbol_table,
  symbols:["another one"]
}

If you call one of the ion_reader methods (for example ion_reader_read_timestamp) with a bad hREADER pointer, the methods fail with IERR_INVALID_STATE, which is misleading. There should be a specific error to indicate that an invalid reader was provided. The current code implies that the method is being called at a bad point in the reader's state-transition model, which isn't the case.

Add tests for reading and writing decimal numbers

For example, when positioned on a null value, ion_reader_read_bool returns IERR_NULL_VALUE when read by a binary reader and IERR_INVALID_STATE when read by a text reader. It should return IERR_NULL_VALUE in this case for all APIs from all readers.

Replace internal build system with cmake

Add tests that round trip every type through the binary format

Via Valgrind.

ion_initialize_page_pool in ion_allocation.c: should we pre-allocate pages when we initialize the page pool?

This parameter is documented as "the number of bytes written into the buffer/stream". Is that the total number of bytes ever written to the stream, or the total number of bytes written to the stream by that call? The behavior appears to be inconsistent between text and binary writers. This should be fixed, and the documentation clarified.

Another option is to deprecate the current flush/finish functions in favor of versions without the output parameter. Users would be directed to use ion_stream_get_position to get the size of the stream. They should do this anyway until this issue is resolved.

Functions like ion_timestamp_for_fraction and ion_timestamp_get_thru_minute aren't necessarily covered extensively by the roundtrip tests.

ion_writer_write_int in ion_writer.c: doesn't support anything larger than 64-bit

The memory page pool used by the text parser isn't thread safe. We can disable it by setting the number of pages to zero (trivially calling through to the built-in memory management functions) or upgrade to C11 and take advantage of atomic operations

The memory ownership contract and lifecycle for an ION_INT is currently not documented, and may be confusing to users. For example, when must ion_int_alloc, ion_int_free, and ion_int_init be used?

Currently, the only implementation of ION_CATALOG is as an in-memory store for ION_SYMBOL_TABLEs. ION_CATALOG should be treated as an interface that allows users to optionally provide lookup logic. This can be used, e.g., to lazily or remotely retrieve shared symbol tables.

_ion_writer_flush_helper in ion_writer.c: should this reset the temp buffer and/or the local symbol table?

In ion_writer_write_one_value, a temp buffer is used to first read the lob and then write it.
This requires two memcpy.

Implementation isn't thread safe (specifically, usage of global static p_system_symbol_table_version_1 in ion_symbol_table.c)

ion_timestamp_to_time_t in ion_timestamp.c: timegm() fails for timestamps before the Unix epoch. It is also unportable!

Add tests to cover all of the possible reader states

#27 pointed out the use of malloc.h in Windows. We should eliminate this if it is not needed (should just use stdlib.h).

Write tests that round trip each type through the text format

Add tests that write out various values and then verify the emitted bits (some overlap with round-tripping)

The ionsymbols tool is untested. If it provides any functionality that is useful to users, it should probably be combined with ionizer.

I tried building ion-c: the debug build works fine (./build-debug.sh) but the release build fails with the error:

[ 71%] Linking CXX executable all_tests
Undefined symbols for architecture x86_64:
  "join_path(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >)", referenced from:
      __GLOBAL__sub_I_test_vectors.cpp in test_vectors.cpp.o
      IonCli_ProcessBasic_Test::TestBody() in test_ion_cli.cpp.o
      IonCli_CompareNullsBasic_Test::TestBody() in test_ion_cli.cpp.o
      IonCli_CompareListsEquivs_Test::TestBody() in test_ion_cli.cpp.o
      IonCli_CompareSexpsNonequivs_Test::TestBody() in test_ion_cli.cpp.o
      IonCli_CompareAnnotatedIvmsEmbeddedNonequivs_Test::TestBody() in test_ion_cli.cpp.o
      IonCli_CompareMultipleInputFiles_Test::TestBody() in test_ion_cli.cpp.o
      ...
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [test/all_tests] Error 1
make[1]: *** [test/CMakeFiles/all_tests.dir/all] Error 2
make: *** [all] Error 2

I'm using the latest master branch (0903060), macOS 10.13.4, the Clang that came with Xcode 9.3, and latest CMake from Homebrew:

$ clang --version
Apple LLVM version 9.1.0 (clang-902.0.39.1)
Target: x86_64-apple-darwin17.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
$ cmake --version
cmake version 3.11.2

ion_temp_buffer_make_utf8_string in ion_writer.c: how do we detect the current platform encoding?

gcov is an option.

The ion reader and writer options declare user-configurable upper bounds for things like maximum annotation count, maximum container depth, etc. This allows memory to be preallocated and reused. Ideally, there would be a similarly-performant solution which does not require upper bounds.

The text writer currently uses sprintf for its float-to-string conversions. Ryu may provide an opportunity to improve both the correctness and performance of this conversion.

From the authors:

We present Ryū, a new routine to convert binary floating point numbers to their decimal representations using only fixed-size integer operations, and prove its correctness. Ryū is simpler and approximately three times faster than the previously fastest implementation.

See also:

• The Github repository, which includes a C implementation.
• A video, which introduces the algorithm.
• The paper, which includes the proof.
• The related ion-java issue.

Laundry list o' platforms:

android
fireos
kindle
ios
macos
windows (WinNTx86-64)
Linux x86-64 (rpm?)

Add coverage for reading and writing the various types of integers (varint, varuint, etc.)

• intptr_t is an integer that is at least as big as the largest pointer. So, the %d is technically fine on i386 GCC (sizeof(intptr_t) == sizeof(int)), but should be %lld on x86_64 GCC (sizeof(intptr_t) == sizeof(int)). Really, we probably should be using the PRIdPTR macro from inttypes.h to be portable, so this is definitely a bug.

      intptr_t val = ...;
      printf("VALUE: %" PRIdPTR  "\n", val);

• The gmtime_r call is a good call out--the CMake C_STANDARD option with 99 uses -std=gnu99.
• We'll have to evaluate the downgrade of CMake and passing the CFLAGS in, I'm not sure this is the right thing to do.
• Consider requiring use of CMake >= 3.5 (note that 3.5 is not available on Ubuntu 14.04).

ion_reader_read_int/int64/long/double has inconsistent behavior when the source data cannot fit into the output variable.

For example, when reading a text int, overflow triggers IERR_NUMERIC_OVERFLOW.
When reading a text double, the value will be truncated.

Consider supporting at least 64 bits.

Making this change will allow good/subfieldVarUInt32bit.ion to be removed from the test skip list.

ion_catalog_find_best_match and ion_catalog_find_symbol_table accept the version parameter as a long, but this is internally converted to an int32_t.

As noted in #94 , non-test users of the CLI should be able to assume that the implementation is bug-free. Therefore, producing two ComparisonResults representing both A == B and B == A is unnecessary noise for these users.

Explore how to best handle this while retaining the utility of not assuming reflexivity for test users. For non-test users, it is probably best to detect reflexive comparisons and only output one ComparisonResult, raising an error if the results of A == B and B == A differ. This might work for test users too.

Error messages are only conveyed in debug builds, and even then, many failure paths do not provide helpful messages. It should be explored how to convey helpful error messages throughout the library, and how to balance any performance implications on release builds.

2000000000d100 roundtrips as 2.000000000d+109 within the event suite.
This is destructive.

_ion_stream_fseek in ion_stream.c: should we validate this cast to long somehow (referring to the invocation of LSEEK)?

The docopt submodule, which generates an argument parser from the CLI help, generates runtime errors on Windows. This makes the CLI executable unusable on that platform.

This needs to be further investigated and fixed.

Need to add C examples to the cookbook

Add tests that read in timestamps and verify the values of individual fields

Known to be not working:

• -o counts
• -o types
• -o none on invalid Ion (should error)

Known to be working:

• -o pretty for the small set of files used to test by hand.
• -o ugly for the small set of files used to test by hand.
• -o binary for the small set of files used to test by hand.
• --help

Unknown: the rest.

The tool needs its own unit testing.

_ion_writer_get_catalog_helper in ion_catalog.c: should we create a catalog if one doesn't already exist?

_ion_hack_bad_value_to_str in ion_helpers.c:

make this thread safe - in some other way - like thread local storage or by taking in user buffers

Certain components of the library, notably ion_extractor's ION_EXTRACTOR_MAX_NUM_PATHS and ION_EXTRACTOR_MAX_PATH_LENGTH, and decNumber's DECNUMDIGITS, allow preprocessor macros to be redefined by the user. Currently, there is no automated way of testing different values for these macros. Such automated testing needs to be added in order to improve coverage.

https://codedocs.xyz/

Integrates with Github and builds documentation using Doxygen by default. We should consider publishing our docs there.