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

Condition → Conditional; IfCondition → Condition #3862

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bolpat
Copy link
Contributor

@Bolpat Bolpat commented Jun 28, 2024

Using IfCondition in the grammar is misleading. It can be used for if, but also for while and switch. On the other hand, Condition, which refers to conditional compilation constructs, isn’t the stuff in parentheses of a static if, debug or version, but the whole static if (…) / debug (…) / version (…); therefore, I replaced it with Conditional.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @Bolpat! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 5, 2024

Condition and Conditional are easy to mix up. The dmd codebase calls IfCondition an AssignCondition, I suggest renaming it to that for consistency.

If there's still need to clarify Condition then, perhaps it can become StaticCondition.

@Bolpat
Copy link
Contributor Author

Bolpat commented Jul 5, 2024

Condition and Conditional are easy to mix up.

I don’t know how you’d mix those up. A condition is something that you evaluate to a boolean to test it.
An IfCondition should be a condition for an if statement specifically, and is immediately a misnomer if it can be used anywhere where no if is present. IMO, Condition is the perfect name, but I’m not married to Conditional. I chose it because it it wasn’t taken and immediately follows the ConditionalDeclaration and ConditionalStatement. Note that there’s also a ConditionalExpression which is unrelated to conditional compilation. (It should probably named ConditionExpression.)

ConditionalCompilationIntroducer instead for the static if/version/debug thing?

The dmd codebase calls IfCondition an AssignCondition, I suggest renaming it to that for consistency.

I’d really like to keep Condition for what’s in if/while/switch parentheses because it perfectly matches what it means to the average programmer.

Using AssignCondition makes sense for the compiler, I guess, as it spends (probably) much more effort on these:

    auto Identifier = Expression
    scope Identifier = Expression
    TypeCtors Identifier = Expression
    TypeCtors? BasicType Declarator = Expression

If you want AssignCondition in the grammar, that would be my suggestion:

-   IfCondition:
+   Condition:
        Expression
+       AssignCondition
+
+   AssignCondition:
        auto Identifier = Expression
        scope Identifier = Expression
        TypeCtors Identifier = Expression
        TypeCtorsopt BasicType Declarator = Expression

Using AssignCondition for the Expression case is weird.

If there's still need to clarify Condition then, perhaps it can become StaticCondition.

But Condition is simply wrong. It refers not the the boolean expression inside parentheses, but the whole “introducer” to the conditional compilation construct, i.e. the whole static if ( AssignExpression ) version(none) or debug(xyz).

@dkorpel
Copy link
Contributor

dkorpel commented Jul 5, 2024

ConditionalCompilationIntroducer

That sounds like a Java name 🙂

the_world_seen_by_an_objectoriented_programmer

But Condition is simply wrong.

static if (xyz) debug(xyz) and version(xyz) are all different conditions to compile the following block, the condition isn't merely xyz.

But that's a bit besides the point. I think it's not worth bikeshedding over the best names for grammar production rules. They are mostly local. It's best to just pick something and be consistent with it. This PR changes them for marginally better clarity at best, so unless the names are made consistent with dmd (which is a nice property as they often need to be compared), I'd err on the side of keeping them as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants