Format function arg defaults from their source text#1069
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1069 +/- ##
==========================================
- Coverage 87.01% 86.87% -0.14%
==========================================
Files 23 23
Lines 6069 6074 +5
==========================================
- Hits 5281 5277 -4
- Misses 788 797 +9
🚀 New features to boost your workflow:
|
| // is not a plain literal. | ||
| errno = 0; | ||
| char* ParseEnd = nullptr; | ||
| double DefaultArgValue = std::strtod(Result.c_str(), &ParseEnd); |
There was a problem hiding this comment.
Maybe we can take a step back here -- is that the way clang does this?
There was a problem hiding this comment.
good point... claude pointed out to me that there is a more standard, generic fix... two possibilities... let me try that them, and repost... might close one of the two PRs actually, since they are related. Give me a sec...
There was a problem hiding this comment.
Good call — reworked. It now does what SemaCodeComplete's GetDefaultValueString does: read the parameter's getDefaultArgRange() source text via Lexer::getSourceText (including stripping the sometimes-present leading =, per the FIXME there), falling back to the AST pretty-print only when there's no usable range. That makes the output as-written by construction, so the float-precision FIXME and the std::stod normalization are gone entirely. Literal defaults now render exactly as written (4.0 where the old code produced 4.) — existing test expectations updated to match.
|
clang-tidy review says "All clean, LGTM! 👍" |
GetFunctionArgDefault pretty-printed the default's AST and round-tripped
floating-typed results through std::stod to undo clang printing floats at
representation precision. A symbolic default (double ratio = kDefaultRatio,
or arrow's pool = default_memory_pool() style) is not a numeric literal, so
stod threw std::invalid_argument into the exception-free build and the
process terminated.
Read the default's source range instead - the way clang's own code
completion formats default arguments (GetDefaultValueString in
SemaCodeComplete): as-written by construction, so the float-precision
FIXME and its numeric normalization disappear. The lexed range sometimes
includes the leading '=' (SemaCodeComplete documents the same quirk), so
strip it; defaults with no usable source range keep the pretty-printed
fallback. Literal defaults now render exactly as written ("4.0" where the
normalized form was "4.").
Co-developed-with-the-help-of: Claude Code (Fable 5, human in the loop)
conrade-ctc
changed the title
189b6aa to
2d07020
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
Description
Cpp::GetFunctionArgDefaultpretty-printed the default's AST and round-tripped floating-typed results throughstd::stod— a FIXME-documented workaround for clang printing floats at representation precision (3.1400000000000001for a written3.14). A symbolic default (double ratio = kDefaultRatio, orMemoryPool* pool = default_memory_pool()-style APIs with float variants, as in arrow/parquet) is not a numeric literal, sostd::stodthrewstd::invalid_argumentinto code built without exception support and the process terminated. Any reflection orhelp()-style prototype formatting of such a method was fatal.Per review, this now formats the default the way clang's own code completion does (
GetDefaultValueStringinSemaCodeComplete): read the parameter'sgetDefaultArgRange()source text viaLexer::getSourceText. That is as-written by construction, so the float-precision FIXME and its numeric normalization disappear entirely. The lexed range sometimes includes the leading=(SemaCodeComplete documents the same quirk), which is stripped; defaults with no usable source range fall back to the previous AST pretty-print.Behavior note: literal defaults now render exactly as written —
4.0where the old normalization produced4.— and the existing test expectations are updated accordingly.Type of change
Testing
Updated
FunctionReflection_GetFunctionArgDefaultfor as-written literal spellings, and newFunctionReflection_GetFunctionArgDefaultSymboliccovering an identifier default, a call default, an expression default, and a literal — the symbolic shapes were fatal before this change.Checklist
🤖 Done with the help of Claude Code (Fable 5, human in the loop)