Skip to content

[clang-repl] Emit const variables only once #65257

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 3, 2023
Merged

[clang-repl] Emit const variables only once #65257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions clang/lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11764,6 +11764,16 @@ GVALinkage ASTContext::GetGVALinkageForFunction(const FunctionDecl *FD) const {

static GVALinkage basicGVALinkageForVariable(const ASTContext &Context,
const VarDecl *VD) {
// As an extension for interactive REPLs, make sure constant variables are
// only emitted once instead of LinkageComputer::getLVForNamespaceScopeDecl
// marking them as internal.
if (Context.getLangOpts().CPlusPlus &&
Context.getLangOpts().IncrementalExtensions &&
VD->getType().isConstQualified() &&
!VD->getType().isVolatileQualified() && !VD->isInline() &&
!isa<VarTemplateSpecializationDecl>(VD) && !VD->getDescribedVarTemplate())
return GVA_DiscardableODR;

if (!VD->isExternallyVisible())
return GVA_Internal;

Expand Down
29 changes: 29 additions & 0 deletions clang/test/Interpreter/const.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// UNSUPPORTED: system-aix
// RUN: cat %s | clang-repl | FileCheck %s
// RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s

extern "C" int printf(const char*, ...);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a case for a file scope constant? We also should consider enabling that for C.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const A a(1); is a file-scope constant, no? We don't need it for C because the special case in LinkageComputer::getLVForNamespaceScopeDecl only applies to C++ (my understanding is that const variables always have an identity in C).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const A a(1); is a file-scope constant, no?

Yes, missed that.

struct A { int val; A(int v); ~A(); void f() const; };
A::A(int v) : val(v) { printf("A(%d), this = %p\n", val, this); }
A::~A() { printf("~A, this = %p, val = %d\n", this, val); }
void A::f() const { printf("f: this = %p, val = %d\n", this, val); }

const A a(1);
// CHECK: A(1), this = [[THIS:0x[0-9a-f]+]]
// The constructor must only be called once!
// CHECK-NOT: A(1)

a.f();
// CHECK-NEXT: f: this = [[THIS]], val = 1
a.f();
// CHECK-NEXT: f: this = [[THIS]], val = 1

%quit
// There must still be no other constructor!
// CHECK-NOT: A(1)

// At the end, we expect exactly one destructor call
// CHECK: ~A
// CHECK-SAME: this = [[THIS]], val = 1
// CHECK-NOT: ~A