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

Class default syntax support #172

Merged
merged 5 commits into from
Feb 2, 2024

Conversation

v0-e
Copy link

@v0-e v0-e commented Jan 22, 2024

Adds support for CLASS default syntax.
This complements the already implemented defined syntax (through the construct WITH SYNTAX {}).

Class instances with no defined syntax, must be implemented using the default syntax.
The default syntax follow the format: "{" FieldSetting "," * "}", where FieldSetting is PrimitiveFieldName Setting.
A PrimitiveFieldName is a reference to a class element, and Setting is a value, type, or object.
FieldSetting's can appear in any order in the class instance definition.

Fixes the compilation of ASN.1 definitions with classes without WITH SYNTAX {}, such as the one in issue #171.

A new test (164) was added.

Current issue: tests related with the 18-class-OK.asn1 file currently fail.
There are several reasons, and they only came to light now because the proposed PR now actually handles the fields inside of classes with default syntax.

  1. a comma is missing after { PosPair | NegPair }. Nokalva's playground also cannot compile this file for the same reason;
  2. now if you define PosPair and NegPair as some types, asn1c still cannot handle the construct { PosPair | NegPair };
  3. the 0 defined for &result-if-error cannot be handled because &ResultType can be any type (with NULL as default).

Where issue 1. should be fixed by changing the ASN definition itself itself, 2. should be fixed in asn1c. This last one is probably related with this warning message, where most FieldSpec's are not yet handled.
About 3., I'm not sure if setting 0 is correct if the underlying type can be any.

I did not yet check how hard or easy it is to fix 2., and I don't know if I'll have the time to look into it.
In the meantime, we can modify/remove 18-class-OK.asn1 to pass the tests.

Followed X.681 (2021) standard.

@mouse07410
Copy link
Owner

In the meantime, we can modify/remove 18-class-OK.asn1 to pass the tests.

You want to split this PR into those that pass CI and can be merged, and those that require more work and will be on hold for now?

@v0-e
Copy link
Author

v0-e commented Jan 22, 2024

Do you mean, e.g., modifying the mentioned test file to pass the tests for now?

@mouse07410
Copy link
Owner

Do you mean, e.g., modifying the mentioned test file to pass the tests for now?

I guess it depends on why it's failing.

If this PR causes a correct and expected change in the output that the current test-file does not account for, then probably yes. If the output just changes and gets out-of-sync with the test-file, then definitely no.

@v0-e
Copy link
Author

v0-e commented Jan 30, 2024

I've modified 18-class-OK.asn1 to remove the class fields not currently supported, namely VariableTypeValueFieldSpec and VariableTypeValueSetFieldSpec.
The related issue affects both default syntax classes and defined syntax classes.

To be clear, the relevant test file is only used in contained unit tests like the fixer tests, and was not being thoroughly parsed/analyzed/tested before the proposed changes.

@mouse07410 mouse07410 merged commit a6bdcd2 into mouse07410:vlm_master Feb 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants