Skip to content

Add implicit IGNORE_IF_UNPOPULATED for MessageOneofRule fields#110

Merged
srikrsna-buf merged 5 commits intosk/oneoffrom
sk/implicit
Jun 13, 2025
Merged

Add implicit IGNORE_IF_UNPOPULATED for MessageOneofRule fields#110
srikrsna-buf merged 5 commits intosk/oneoffrom
sk/implicit

Conversation

@srikrsna-buf
Copy link
Copy Markdown
Contributor

Signed-off-by: Sri Krishna <skrishna@buf.build>
@srikrsna-buf srikrsna-buf requested review from a user and timostamm June 13, 2025 06:55
Comment thread buf/validate/internal/message_rules.cc
Signed-off-by: Sri Krishna <skrishna@buf.build>
Signed-off-by: Sri Krishna <skrishna@buf.build>
Comment thread buf/validate/internal/message_rules.cc Outdated
}
fields.push_back(fdesc);
fields.push_back(fdesc);
allMsgOneofs.insert(name) ;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
allMsgOneofs.insert(name) ;
allMsgOneofs.insert(name);

Something odd happened to the formatting on this line.

Comment thread buf/validate/internal/message_rules.cc Outdated
FieldRules fieldLvlOvr;
fieldLvlOvr.CopyFrom(fieldLvl.get());
fieldLvlOvr.set_ignore(IGNORE_IF_UNPOPULATED);
fieldLvl = fieldLvlOvr;
Copy link
Copy Markdown

@ghost ghost Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to me like it would rebind the std::reference_wrapper to a pointer to fieldLvlOvr whose lifetime ends after this scope. This may actually work but I think it is undefined behavior because the stack space is not guaranteed to be clobbered before it is read/copied. Maybe I am wrong?

I think the canonical way to do this safely is to allocate a new FieldRules in the current arena.

Signed-off-by: Sri Krishna <skrishna@buf.build>
Signed-off-by: Sri Krishna <skrishna@buf.build>
@srikrsna-buf srikrsna-buf merged commit e1c9b7c into sk/oneof Jun 13, 2025
3 checks passed
@srikrsna-buf srikrsna-buf deleted the sk/implicit branch June 13, 2025 17:01
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