Page MenuHomePhabricator

Do not evaluate dependent immediate invocations
ClosedPublic

Authored by usaxena95 on Aug 17 2022, 8:01 AM.

Details

Summary

We deferred the evaluation of dependent immediate invocations in https://reviews.llvm.org/D119375 until instantiation.
We should also not consider them referenced from a non-consteval context.

Fixes: https://github.com/llvm/llvm-project/issues/55601

template<typename T>
class Bar {
  consteval static T x() { return 5; }
 public:
  Bar() : a(x()) {}

 private:
  int a;
};

Bar<int> g();

Is now accepted by clang. Previously it errored with: cannot take address of consteval function 'x' outside of an immediate invocation Bar() : a(x()) {}

Diff Detail

Event Timeline

usaxena95 created this revision.Aug 17 2022, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 8:01 AM
usaxena95 requested review of this revision.Aug 17 2022, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 8:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
usaxena95 edited the summary of this revision. (Show Details)Aug 17 2022, 8:02 AM
usaxena95 edited the summary of this revision. (Show Details)
usaxena95 edited the summary of this revision. (Show Details)
aaron.ballman accepted this revision.Aug 17 2022, 9:31 AM

Thank you, this LGTM, though I did have a testing question. Also, please be sure to add a release note for the fix.

clang/test/SemaCXX/cxx2a-consteval.cpp
664

Do you think it's worth it to add a test case like:

struct derp {
  derp(int); // Can't be used in a constant expression

  consteval operator int() const { return 5; }
};

Bar<derp> d; // Error

to demonstrate that we evaluated properly at instantiation time, or do you think existing coverage already demonstrates this well enough?

This revision is now accepted and ready to land.Aug 17 2022, 9:31 AM
usaxena95 marked an inline comment as done.
usaxena95 edited the summary of this revision. (Show Details)

Addressed comments.

Addressed comments. Added release note.

clang/test/SemaCXX/cxx2a-consteval.cpp
664

Added the test.

A few NITS for the release notes, otherwise LG

clang/docs/ReleaseNotes.rst
162

NIT: a small nit

163

NIT: to align with release notes above

usaxena95 updated this revision to Diff 453562.Aug 18 2022, 1:28 AM
usaxena95 marked 2 inline comments as done.

Updated release notes.

This revision was landed with ongoing or failed builds.Aug 18 2022, 1:31 AM
This revision was automatically updated to reflect the committed changes.