Skip to content

Format function arg defaults with the constants-as-written printer #1069

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

Open
conrade-ctc wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Format function arg defaults with the constants-as-written printer #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1353072
Format function arg defaults with the constants-as-written printer
Jul 16, 2026
5c372a8
Add a canary test for clang's float pretty-printing
Aug 24, 2026
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
44 changes: 30 additions & 14 deletions lib/CppInterOp/CppInterOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Demangle/Demangle.h"
#include "llvm/ExecutionEngine/JITSymbol.h"
Expand Down Expand Up @@ -6006,6 +6007,11 @@ std::string GetFunctionArgDefault(ConstFuncRef func, size_t param_index) {
PI = (FD->getTemplatedDecl())->getNonObjectParameter(param_index);

if (PI->hasDefaultArg()) {
// Print the AST with ConstantsAsWritten and the interpreter's ASTContext:
// literal leaves with valid source ranges then render from source text
// ("3.14", not the representation-precision "3.1400000000000001"). The
// previous std::stod normalization terminated the exception-free build on
// symbolic defaults such as `double ratio = kDefaultRatio`.
std::string Result;
llvm::raw_string_ostream OS(Result);
const Expr* DefaultArgExpr = nullptr;
Expand All @@ -6014,20 +6020,30 @@ std::string GetFunctionArgDefault(ConstFuncRef func, size_t param_index) {
DefaultArgExpr = PI->getUninstantiatedDefaultArg();
else
DefaultArgExpr = PI->getDefaultArg();
DefaultArgExpr->printPretty(OS, nullptr, PrintingPolicy(LangOptions()));

// FIXME: Floats are printed in clang with the precision of their underlying
// representation and not as written. This is a deficiency in the printing
// mechanism of clang which we require extra work to mitigate. For example
// float PI = 3.14 is printed as 3.1400000000000001
if (PI->getType()->isFloatingType()) {
if (!Result.empty() && Result.back() == '.')
return INTEROP_RETURN(Result);
auto DefaultArgValue = std::stod(Result);
std::ostringstream oss;
oss << DefaultArgValue;
Result = oss.str();
}
ASTContext& Ctx = getASTContext();
PrintingPolicy Policy(Ctx.getLangOpts());
Policy.ConstantsAsWritten = true;

@vgvassilev vgvassilev Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, that’s nice. And probably kills our upstream story probably for good.

@conrade-ctc conrade-ctc Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@vgvassilev, do you think the UDL hole that is listed above is worth a PR to clang as well? it's a separate gap that might be valuable to clean up.

@vgvassilev vgvassilev Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, ok. Did not realize that’s a disk access. That’s pretty slow for the pch case and requires headers to be around..

@conrade-ctc conrade-ctc Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

hmmm, by UDL i mean user-defined-literal, not the disk one :) Just want to make sure we're on the same page... there is apparently a minor hole in the UDL float impl that mixes 90.0_deg and 90._deg for example... claude identified that as a follow up that is related and needs a separate treatment upstream.

DefaultArgExpr->printPretty(OS, nullptr, Policy, /*Indentation=*/0,
/*NewlineSymbol=*/"\n", &Ctx);

// CPyCppyy evaluates numeric defaults in Python after stripping
// uppercase literal suffixes, so print the suffix in its canonical
// uppercase form ("5.f" -> "5.F"). The per-kind alphabets keep hex
// digits safe: 'f' in 0x1f is a digit of an IntegerLiteral, whose
// suffix alphabet has no 'f'.
const Expr* Leaf = DefaultArgExpr->IgnoreImpCasts();
if (const auto* UO = llvm::dyn_cast<clang::UnaryOperator>(Leaf))
Leaf = UO->getSubExpr()->IgnoreImpCasts();
const char* SuffixAlphabet = nullptr;
if (llvm::isa<IntegerLiteral>(Leaf))
SuffixAlphabet = "uUlLzZ";
else if (llvm::isa<FloatingLiteral>(Leaf))
SuffixAlphabet = "fFlL";
if (SuffixAlphabet)
for (size_t I = Result.find_last_not_of(SuffixAlphabet) + 1;
I < Result.size(); ++I)
Result[I] = llvm::toUpper(Result[I]);

return INTEROP_RETURN(Result);
}
return INTEROP_RETURN("");
Expand Down
73 changes: 70 additions & 3 deletions unittests/CppInterOp/FunctionReflectionTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
#include "CppInterOp/CppInterOp.h"

#include "clang/AST/ASTContext.h"
#include "clang/AST/PrettyPrinter.h"
#include "clang/Basic/Version.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Sema/Sema.h"

#include <CppInterOp/CppInterOpTypes.h>
#include <cstdint>
#include <llvm/ADT/ArrayRef.h>
#include <llvm/Support/raw_ostream.h>

#include "gtest/gtest.h"

Expand Down Expand Up @@ -3721,10 +3723,10 @@ TYPED_TEST(CPPINTEROP_TEST_MODE, FunctionReflection_GetFunctionArgDefault) {
GetAllTopLevelDecls(code, Decls);

EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[0], 0), "");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[0], 1), "4.");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[0], 1), "4.0");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[0], 2), "\"default\"");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[0], 3), "\'c\'");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[1], 0), "0.");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[1], 0), "0.0");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[1], 1), "3.123");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[1], 2), "34126");

Expand All @@ -3739,7 +3741,7 @@ TYPED_TEST(CPPINTEROP_TEST_MODE, FunctionReflection_GetFunctionArgDefault) {
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[4], 0), "");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[4], 1), "");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[4], 2), "\'a\'");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[4], 3), "0.");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[4], 3), "0.0");

ASTContext& C = Interp->getCI()->getASTContext();
std::vector<Cpp::TemplateArgInfo> template_args = {C.IntTy.getAsOpaquePtr()};
Expand All @@ -3754,6 +3756,71 @@ TYPED_TEST(CPPINTEROP_TEST_MODE, FunctionReflection_GetFunctionArgDefault) {
EXPECT_EQ(Cpp::GetFunctionArgDefault(fn, 1), "S()");
}

TYPED_TEST(CPPINTEROP_TEST_MODE,
FunctionReflection_GetFunctionArgDefaultSymbolic) {
std::vector<Decl*> Decls;
std::string code = R"(
constexpr double kDefaultRatio = 0.5;
double default_ratio();
double scaled(double ratio = kDefaultRatio);
double rescaled(double ratio = default_ratio());
double inverted(double ratio = 2.0 / kDefaultRatio);
double pi_ish(double p = 3.14);
float take_float(float a = 5.f);
long take_long(long a = -5l);
unsigned long take_ulong(unsigned long a = 5ul);
int take_hex(int a = 0x1f);
)";

GetAllTopLevelDecls(code, Decls);

// A floating-typed default need not be a numeric literal. Formatting a
// symbolic default must not crash (the removed std::stod normalization
// terminated the exception-free build) and must render it as written.
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[2], 0), "kDefaultRatio");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[3], 0), "default_ratio()");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[4], 0), "2.0 / kDefaultRatio");
// Literals render exactly as written, not at representation precision.
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[5], 0), "3.14");
// Numeric-literal suffixes render uppercase: cppyy strips only that form
// before it evaluates the default in Python ("5.f" is a Python syntax
// error, "5.F" strips to "5."). Hex digits are not suffixes.
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[6], 0), "5.F");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[7], 0), "-5L");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[8], 0), "5UL");
EXPECT_EQ(Cpp::GetFunctionArgDefault(Decls[9], 0), "0x1f");
}

TYPED_TEST(CPPINTEROP_TEST_MODE,
FunctionReflection_FloatingDefaultPrinterCanary) {
#ifdef EMSCRIPTEN
GTEST_SKIP() << "The wasm test binary does not link the raw clang printer "
"symbols (printPretty, getDefaultArg) this test needs.";
#else
std::vector<Decl*> Decls;
GetAllTopLevelDecls("void canary(double x = 3.14);", Decls);

// Before clang 24 the raw printer expands floats to maximum precision;
// UDL and invalid-range defaults hit it. llvm/llvm-project#218471 fixes
// the printer for clang 24. Each branch failing is a signal: see its
// message.
const auto* PD = cast<FunctionDecl>(Decls[0])->getParamDecl(0);
std::string Raw;
llvm::raw_string_ostream OS(Raw);
Comment thread
conrade-ctc marked this conversation as resolved.
PD->getDefaultArg()->printPretty(OS, nullptr, PrintingPolicy(LangOptions()));
Comment thread
conrade-ctc marked this conversation as resolved.
#if CLANG_VERSION_MAJOR < 24
EXPECT_EQ(Raw, "3.1400000000000001")
<< "clang's pretty-printer round-trips floating literals earlier than "
"expected (llvm/llvm-project#218471 cherry-picked?). Re-check UDL "
"and invalid-range defaults, then move this guard.";
#else
EXPECT_EQ(Raw, "3.14")
<< "llvm/llvm-project#218471 did not land in clang 24. Raise the "
"version in this guard.";
#endif
#endif // EMSCRIPTEN
}

TYPED_TEST(CPPINTEROP_TEST_MODE, FunctionReflection_Construct) {
#ifdef _WIN32
GTEST_SKIP() << "Disabled on Windows. Needs fixing.";
Expand Down
Loading