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

Support Implied Keys #1339

Merged
merged 14 commits into from Dec 9, 2019
Merged

Conversation

iguessthislldo
Copy link
Member

  • Support Implied Keys:
    • If a nested struct has no specified keys and a field of the nested struct type is marked with @key, all the fields in the nested struct are implied to be marked with @key.
  • opendds_idl will now error if the set IDL version is less than 4.
  • Added topic annotation migration guide and updated deprecation warning and news file to point to it.

When marking @key on a struct field, if the field type is a struct with
no fields marked as @key, then all the fields will be assumed to be a
key in that context.
.gitignore Outdated Show resolved Hide resolved
dds/idl/be_init.cpp Outdated Show resolved Hide resolved
dds/idl/topic_keys.cpp Outdated Show resolved Hide resolved
dds/idl/topic_keys.cpp Outdated Show resolved Hide resolved
@iguessthislldo
Copy link
Member Author

Another question: do implied keys apply to unions? If we're being consistent I think these rules should apply to union discriminators as well:

If a union field or array field in a struct is marked as a key or implied to be a key, then the discriminator is assumed to be the key unless marked with @key(FALSE).

If there are fields that are marked with @key(FALSE) and none marked
with @key or @key(TRUE), then keys can still be implied, but it excludes
the fields that are marked with @key(FALSE).
@iguessthislldo
Copy link
Member Author

I wonder if the Github user key has blocked me by now.

@iguessthislldo
Copy link
Member Author

Another thought following my previous ones: If a user has marked a field in a struct of another type (union or struct) and all the possible keys have been marked with @key(FALSE), what does that mean? Is it an error or a possible chance to warn the user? Right now I think it would error with structs, which is the behavior before this PR.

@mitza-oci
Copy link
Member

I wonder if the Github user key has blocked me by now.

Right... I'm trying to use @key formatting but I may forget sometimes.

@mitza-oci
Copy link
Member

Another question: do implied keys apply to unions? If we're being consistent I think these rules should apply to union discriminators as well:

If a union field or array field in a struct is marked as a key or implied to be a key, then the discriminator is assumed to be the key unless marked with @key(FALSE).

I could see it going either way. The other argument would be that the semantics of @key in the outer struct seem to be saying "use this whole thing as a key," and that doesn't apply to the union example.

@iguessthislldo
Copy link
Member Author

So I'm kinda confused. I changed the PR Jenkins configure step like 2 days ago to use --ace-github-latest, because it couldn't compile opendds_idl, but it's still acting like it doesn't have ACE/TAO #967 and giving the same error...

@iguessthislldo
Copy link
Member Author

I could see it going either way. The other argument would be that the semantics of @key in the outer struct seem to be saying "use this whole thing as a key," and that doesn't apply to the union example.

I wasn't thinking of it applying it to a struct necessarily, I was thinking of applying it directly to the union field like so:

union UnionType switch (int) {
// ...
};

@topic
struct StructType {
  @key UnionType union_value;
};

Although I think it also apply indirectly as well if the union is implied to be a key:

union UnionType switch (int) {
// ...
};

struct InnerStructType {
  UnionType union_value;
};

@topic
struct OuterStructType {
  @key InnerStructType inner_struct_value;
};

@mitza-oci
Copy link
Member

I could see it going either way. The other argument would be that the semantics of @key in the outer struct seem to be saying "use this whole thing as a key," and that doesn't apply to the union example.

I wasn't thinking of it applying it to a struct necessarily, I was thinking of applying it directly to the union field like so:

union UnionType switch (int) {
// ...
};

@topic
struct StructType {
  @key UnionType union_value;
};

This is exactly the one I was referring to. Does it make sense to say that "union_value is the key of StructType"? It's not really, only the discriminator can be a key so the union value as a whole isn't.

@iguessthislldo
Copy link
Member Author

This is exactly the one I was referring to. Does it make sense to say that "union_value is the key of StructType"? It's not really, only the discriminator can be a key so the union value as a whole isn't.

Yes, of course you can't use the whole union value, but it's always possible use the discriminator as a key, so it would be used unless the IDL writer says explicitly not to using @key(FALSE). Since this isn't none of this actually standardized I'm just taking the opportunity to try to clarify how this should work and find out what the limits, if any, are there on implied keys. My takeaway from RTI's documentation is that they will use as much as possible as a key. However, I should add that the documentation actually doesn't say what would happen in this situation, so ideally I would like this brought up with the rest of the spec issue on implied keys.

dds/idl/be_global.h Outdated Show resolved Hide resolved
dds/idl/be_global.cpp Outdated Show resolved Hide resolved
dds/idl/be_init.cpp Show resolved Hide resolved
dds/idl/topic_keys.cpp Outdated Show resolved Hide resolved
dds/idl/topic_keys.cpp Outdated Show resolved Hide resolved
dds/idl/topic_keys.cpp Outdated Show resolved Hide resolved
tests/DCPS/Compiler/key_annotation/main.cpp Outdated Show resolved Hide resolved
iguessthislldo added a commit to iguessthislldo/OpenDDS that referenced this pull request Dec 2, 2019
Update guidelines based on cases such
OpenDDS#1379 (comment)
and
OpenDDS#1339 (comment)

Removed bit in Doxygen, because it doesn't really work like I thought it
did.
@iguessthislldo iguessthislldo mentioned this pull request Dec 2, 2019
@mitza-oci mitza-oci merged commit f658015 into OpenDDS:master Dec 9, 2019
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.

None yet

2 participants