Skip to content

Conversation

@wismill
Copy link
Member

@wismill wismill commented Nov 20, 2025

Fail whenever an unsupported flag is used. See the discussion in #932

See also #942 to check valid flags properly.

@bluetech @whot this is probably better to fail explicitly (and early) rather than lie to the user.

It also enable indirectly to check for supported flags.

TODO:

  • Single changelog entry
  • Test all the functions with unsupported flags

@wismill wismill added this to the 1.14.0 milestone Nov 20, 2025
@wismill wismill force-pushed the context/reject-unsupported-flags branch from 425e801 to 7be67e9 Compare November 20, 2025 14:40
@bluetech
Copy link
Member

There is a problem that this change is in itself a breaking change. Might be fine though?

@wismill
Copy link
Member Author

wismill commented Nov 28, 2025

I agree, but it seems the only proper behavior (see the description). Currently some functions follow this behavior and other do not. This MR makes the behavior consistent across the code base.

@wismill wismill force-pushed the context/reject-unsupported-flags branch 2 times, most recently from 87ac7f4 to b5ff4e9 Compare December 2, 2025 17:26
@wismill wismill marked this pull request as ready for review December 2, 2025 17:26
Copy link
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

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

There is a problem that this change is in itself a breaking change. Might be fine though?

IMO it's fine - functions not checking their arguments is not something a caller should be relying on.

Code LGTM though I think the various enums are unnecessary (see my inline comments). Thanks for fixing this.


found_path:
ok = parse_file(table, file, path);
{} /* Label followed by a declaration is a C23 extension */
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work?

found_path:
    {
        const bool ok ...
        flcose
        if (!ok) {
        }
    }

Copy link
Member Author

@wismill wismill Dec 9, 2025

Choose a reason for hiding this comment

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

It will work, it’s just that I have some hope to switch to C23 for the basic features: constexpr and attributes would be great appart lifting this label restriction!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave this up to you then, whichever you prefer. all LGTM now, thanks

@wismill wismill force-pushed the context/reject-unsupported-flags branch from b5ff4e9 to c9231a2 Compare December 9, 2025 07:41
@wismill
Copy link
Member Author

wismill commented Dec 9, 2025

Rebased & applied review

@wismill wismill force-pushed the context/reject-unsupported-flags branch from c9231a2 to 3d56a18 Compare December 10, 2025 17:05
@wismill wismill merged commit ccb63bf into xkbcommon:master Dec 10, 2025
6 checks passed
@wismill wismill deleted the context/reject-unsupported-flags branch December 10, 2025 17:30
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.

3 participants