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

[BUG] Variables in "block scope parameter" declarations should not default to in (const) #1000

Closed
bluetarpmedia opened this issue Feb 27, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@bluetarpmedia
Copy link
Contributor

bluetarpmedia commented Feb 27, 2024

Describe the bug
(Apologies if this should be a suggestion rather than a bug!)

I want to write a loop and also use a counter, but I want the counter variable to be scoped with the loop, and not accessible outside. When I use the "block scope parameter" feature (not sure if that's the correct name!) the variable defaults to in and is therefore const, so I can't update the counter inside the loop.

To Reproduce
Run cppfront on this code:

main: () -> int = {
    words: std::vector<std::string> = ("Adam", "Betty");

    (i: int = 0)
    for words do (word)
    {
        std::cout << i++ << ": " << word << "\n";
    }

    // I don't want access to `i` here

    return 0;
}

See Godbolt.

The same applies when using the next syntax:

(i: int = 0)
for words next i++ do (word)

The relevant code lowers to:

{
  cpp2::in<int> i{0};
  for ( auto const& word : std::move(words) ) 
  {
    std::cout << ++i << ": " << word << "\n";
  }
}

and causes a C++ compiler error: cannot assign to variable 'i' with const-qualified type 'cpp2::in<int>' (aka 'const int')

My first thought was to try inout since I wanted to mutate i, but that doesn't work (see below).

I get the desired behaviour by writing:

(copy i: int = 0)
for ...

The 3 currently supported options are:

Syntax Lowers to Compiles?
in (default) cpp2::in<int> i{0}; No
copy int i{0}; Yes
inout int& i{0}; No

Desired behaviour
There's a lot to like about unifying functions and local/block scope parameters, but I think variables declared in the "block scope parameter" list should have different defaults to function parameters.

My reasoning:

  1. The purpose is different. When declaring a variable in a block scope parameter list, the intention is to declare a local variable that has a specific scope, as opposed to declaring a parameter that refers to a caller's argument.
  2. Cpp2 local variables default to mutable. If they defaulted to const then the current block scope parameter behaviour would be consistent with them, but as it is there's a difference.
  3. Having to write copy is not intuitive and also reads awkwardly.
@bluetarpmedia bluetarpmedia added the bug Something isn't working label Feb 27, 2024
@hsutter
Copy link
Owner

hsutter commented Feb 27, 2024

Thanks! I hear you... I've switched the default for statement scope parameters between in and copy a couple of times, and I appreciate more usage experience-based feedback like this.

The most recent switch to the in default (and requiring explicit copy) is in this commit where you can see the effect in 7 places in reflect.h2 as it was then: d215985#diff-9124d88032e1de131eaa643c46e63344e15dd90e07eea20b2c0fcc5e3b9c84d5

See also the terse commit comments on that comment.

But I've continued to hit this myself, where I have to be reminded to write copy.

I'm not sure how to attach a poll to this issue... I'd love to get a quick poll of how many people think in vs copy should be the default. Absent a poll, perhaps it's time to switch back.

@bluetarpmedia
Copy link
Contributor Author

bluetarpmedia commented Feb 27, 2024

GitHub Discussions support polls but I think you have to enable them.

@hsutter
Copy link
Owner

hsutter commented Feb 27, 2024

Ah, I finally found it -- enabled, thanks.

@hsutter
Copy link
Owner

hsutter commented Feb 27, 2024

#1001

@hsutter
Copy link
Owner

hsutter commented Mar 21, 2024

Thanks, everyone! Based on the results of #1000 I'll close this as "status quo" for now, and we can always reopen it again in the future if we get new information.

@hsutter hsutter closed this as completed Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants