icerpc / slicec

The Slice compiler library
Apache License 2.0
13 stars 5 forks source link

Rename encoding keyword to mode #631

Closed externl closed 1 year ago

externl commented 1 year ago

This PR renames the encoding keyword to mode.

In a followup PR we should consider going even deeper and consider renaming SupportEncodings and maybe even Encoding itself to mode?

Fixes #629

bernardnormier commented 1 year ago

I think this PR should rename file_encoding, supported_encodings (etc) and more to mode.

A more apt question is: where should we keep the term 'encoding' in slicec?

pepone commented 1 year ago

We use supported_encodings mostly for writing encoding conditional code, like in:

let mut decode_body = EncodingBlockBuilder::new(
        "decoder.Encoding",
        &struct_def.escape_identifier(),
        struct_def.supported_encodings(),
        false, // No encoding check for structs
    )
    .add_encoding_block(Encoding::Slice1, || {
        decode_fields(&fields, &namespace, FieldType::NonMangled, Encoding::Slice1)
    })
    .add_encoding_block(Encoding::Slice2, || {
        decode_fields(&fields, &namespace, FieldType::NonMangled, Encoding::Slice2)
    })
    .build();

I think it makes sense to keep supported_encodings, for example, you can have a Slice file with mode=Slice1 but some definitions in this file support the Slice1 and Slice2 encodings, which to me seems a clear model.

If we rename it to supported_modes, We have a Slice file with mode=Slice1, where some definitions support both the Slice1 and Slice2 compilation modes, and then we generate conditional encoding code for each of the supported compilation modes.

I kind of prefer the first model, with supported encodings

InsertCreativityHere commented 1 year ago

I think it depends on the situation.

file_encoding should become file_mode for sure. This field stores what is written in the encoding = Slice1 statement. Now that the keyword is mode, it should follow suit.

But supported_encodings should stay supported_encodings. This field stores what Slice encodings a construct can encoded with. It's influenced by the file mode, but it really and truly is about the Slice encoding.

InsertCreativityHere commented 1 year ago

A more apt question is: where should we keep the term 'encoding' in slicec?

I think we should only keep it for things that are only related to Slice encodings like:

InsertCreativityHere commented 1 year ago

Maybe we should add a:

pub type Mode = Encoding;

Where we define the Encoding enum.

This way we can write both Mode::Slice1 and Encoding::Slice1 depending on which is more correct in context.

externl commented 1 year ago

Since I'm on vacation for the next week I've assigned @InsertCreativityHere to continue work on this PR :)

InsertCreativityHere commented 1 year ago

Since I'm on vacation for the next week I've assigned @InsertCreativityHere to continue work on this PR :)

No problemo! Enjoy your vacation : v)

InsertCreativityHere commented 1 year ago

@bernardnormier

A more apt question is: where should we keep the term 'encoding' in slicec?

After this change there are only 4 places in the compiler where 'encoding' is still mentioned:

Everywhere else has been renamed from encoding to mode. Usually I tried to qualify it either as compilation mode (when talking in general) or SliceN mode (for a specific mode). This is because we also have 'mode's in the lexers and parsers.