Skip to content

[Frontend] Convert NLOptions to an enum class#89992

Open
filip-sakel wants to merge 2 commits into
swiftlang:mainfrom
filip-sakel:nloptions-enum
Open

[Frontend] Convert NLOptions to an enum class#89992
filip-sakel wants to merge 2 commits into
swiftlang:mainfrom
filip-sakel:nloptions-enum

Conversation

@filip-sakel

@filip-sakel filip-sakel commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Converts the NLOptions enum to an enum class. From my understanding, the codebase wants to use more namespaced enums in general. More particularly, this change allows us to expose NLOptions to ASTGen (by including it in ASTBridging.h), which I need to validate the upcoming qualified-lookup API in swift-syntax.

Changes at the call site:

  1. All NL_[...] constants become NLOptions::[...]
  2. We have a new NLOptions::None to indicate the absence of name-lookup options
  3. NLOptions doesn't implicitly convert to bool. So if you want to test if the given options include a specific options, you convert:
    - bool includesOptionA = options & NL_OptionA;
    + bool includesOptionA = (options & NLOptions::OptionA) != NLOptions::None; 


/// Constants used to customize name lookup.
enum NLOptions : unsigned {
enum class NLOptions : unsigned {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have you considered making this into a NLFlag instead and declaring NLOptions to be an OptionSet? This is a very common pattern in the code-base and helps us avoid declaring plural names as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that. Just one question, does OptionSet depend on the C++ standard library? Because according to BasicBridging.h:

// Do not add other std/llvm header files here!
// Function implementations should be placed into BasicBridging.cpp and
// required header files should be added there.
//
// Pure bridging mode does not permit including any std/llvm headers.
// See also the comments for `BRIDGING_MODE` in the top-level CMakeLists.txt
// file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OptionSet is a defined in swift it’S effectively just a wrapper over a bit field.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I'll change it to an OptionSet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please re-request the review once you are ready to make sure that I don't lose track of this.

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