Skip to content

Conversation

@PankajBhojwani
Copy link
Contributor

Summary of the Pull Request

Compiler was raising the error: expression did not evaluate to a constant., causing the solution to fail when building. Removing this constexpr fixes it.

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Validation Steps Performed

Solution builds now

@DHowett
Copy link
Member

DHowett commented Nov 9, 2022

Huh. This was added for the audit mode build. Isn't this gonna make Audit fail?

Is this caused by a VS version disparity?

@lhecker
Copy link
Member

lhecker commented Nov 9, 2022

The default constructor for std::vector is constexpr since C++20. This should work on any conforming C++20 compiler and unless I did something wrong, it did work in VS 17.3.6.

@lhecker
Copy link
Member

lhecker commented Nov 9, 2022

For reference: https://godbolt.org/z/5qq667fq1

@PankajBhojwani
Copy link
Contributor Author

Got on a call with @lhecker and turns out this repros on his machine as well (with VS 17.3.6)

@lhecker
Copy link
Member

lhecker commented Nov 9, 2022

I figured it out... The debug version of std::vector allocates a 1-item backing storage even in the default constructor. The release one doesn't.

@DHowett
Copy link
Member

DHowett commented Nov 9, 2022

Is it worth doing this?

static
#ifndef NDEBUG
constexpr
#else
const
#endif
std::vector foobarbaz{};

?

That way we keep the optimization on release without introducing a TLS storage check to make sure no two threads enter at the same time?

Or... is that moot because we're returning a const ref anyway?

@lhecker
Copy link
Member

lhecker commented Nov 9, 2022

Or... is that moot because we're returning a const ref anyway?

I think it's moot because constexpr doesn't mean it's constexpr anyways. That's what consteval is for. 🥴 So I'm fairly confident that no matter whether we write constexpr or not, most of the time it should compile to the exact same thing.

@carlos-zamora
Copy link
Member

@msftbot merge this in 10 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 9, 2022
@ghost
Copy link

ghost commented Nov 9, 2022

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Wed, 09 Nov 2022 21:20:23 GMT, which is in 10 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit 7aa7ce2 into main Nov 9, 2022
@ghost ghost deleted the dev/pabhoj/fix_main branch November 9, 2022 21:21
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Marked for automatic merge by the bot when requirements are met

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants