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

[expr.new] Harmonize rules of constant array bounds > 0 #2327

Merged
merged 1 commit into from Jun 17, 2019

Conversation

k-satoda
Copy link
Contributor

@k-satoda k-satoda commented Sep 8, 2018

While "shall evaluate to a strictly positive value" is not wrong,
"shall be greater than zero" is easier and is used in [dcl.array] and
[dcl.init.aggr] to describe the same rule.

As a side note, I was surprized that negative bounds are not actually
banned by these "shall be greater than zero". It is now banned by a
narrowing conversion involved in a "converted constant expression of
type std::size_t" since approval of N3323(390dd71#diff-baa06f0d56d1de4d541e8b3fca69df74).
Now these "shall be greater than zero" actually ban only zero.
But I think changing them to "shall not be zero" is not good (can be
more confusing).

@jensmaurer
Copy link
Member

I agree with the change, but I disagree with the side note. The previous wording used "integral constant expression", which could have been of a signed integer type and therefore would have allowed for negative values. The "converted constant expression of type std::size_t" indeed relies on the absence of narrowing, and I believe the wording (before and after) correctly requires a diagnostic for "new int[-1]", for instance.

@k-satoda
Copy link
Contributor Author

k-satoda commented Sep 8, 2018

I don't see in what point you disagree. To clarify what I said, let me
add more context how I came to these wordings.

My naive reading was "Huh? Evaluating -1 as std::size_t will yields a
value greater than zero, and the rule is broken?" I realized that was
wrong after testing the following:

struct S { constexpr operator int () const { return -1; } };
int a[S()];

gcc 8.2.0 says "error: size of array 'a' is negative". Hmm.
https://wandbox.org/permlink/fjNFyWdK5kOJ9Yfw
gcc HEAD 9.0.0 201809 says "error: narrowing conversion of '-1' from 'int' to 'long unsigned int'".
https://wandbox.org/permlink/YbonMCznxVuqThV0
Then I found how "converted constant expression of type T" is defined
and realized that gcc HEAD is more precise.

@jensmaurer
Copy link
Member

jensmaurer commented Sep 9, 2018

The spelling of error messages is not in scope for the standard.
But I'm now seeing where you're coming from: Since application of N3323, the effect of the phrasing "shall be greater than zero" (previously necessary due to "integral constant expression") is now equivalent to "not equal to zero", and the negative numbers are covered by the narrowing prohibition in "converted constant expression of type size_t". That said, I do agree that "greater than zero" should stay.

@zygoloid
Copy link
Member

zygoloid commented Oct 9, 2018

Putting this on hold: I'd like to either resolve the more-substantial #2274 first, or merge this into that.

@jensmaurer
Copy link
Member

This change is subsumed by #2274.

@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 1, 2018

Let's close this and concentrate on #2274 then.

@jensmaurer
Copy link
Member

After all, this is not covered by #2274.

@jensmaurer jensmaurer reopened this Jun 14, 2019
@jensmaurer jensmaurer added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jun 14, 2019
While "shall evaluate to a strictly positive value" is not wrong,
"shall be greater than zero" is easier and is used in [dcl.array] to
describe the same rule.
@k-satoda k-satoda force-pushed the harmonize-array-bounds-rule branch from 16f1ec6 to 8226cbd Compare June 15, 2019 08:00
@k-satoda
Copy link
Contributor Author

Rebased.
Also I updated the log message since the occurrence of
"shall be greater than zero" in [dcl.init.aggr], which was mentioned in
the initial version of this PR, had been removed in commit
5ef7f9c.

@zygoloid zygoloid removed the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jun 17, 2019
@zygoloid zygoloid merged commit f8b8c25 into cplusplus:master Jun 17, 2019
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.

None yet

4 participants