AaronBallman wrote: > Oh, wow, @AaronBallman, in adding a barrage of tests, I found some > pre-existing I found 3 existing bugs in `APFloat` > > > > Generally LGTM though I did not check the implementation of the algorithm > > > very carefully. The results look correct to me in terms of the testing > > > changes, but I think there are some tests we should add: > > > ``` > > > double bar = 0x10.1p0; > > > ``` > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > do we correctly round-trip that or do we still convert that to a decimal > > > value instead of the hex literal? > > > Another question would be: do we have sufficient test coverage for some > > > of the odder floating-point formats like double double or all the tiny > > > floating point types for AI? > > > > > > Good point. I'll add a barrage of corner case tests, compiling a list now, > > and I'll ping when they are pushed to the PR. > > @AaronBallman, radix is not preserved in round-trip... that requires a bit > more surgery (need radix field in AST nodes, etc). Okay with you to post an > issue for that one, and follow-up with a separate PR, or do you think it's > worth adding to this PR?
A follow-up is totally fine, and thank you for offering to post an issue on it! > Also, glad you pressed on wider test coverage, other that the existing bug > @efriedma-quic stressed in the comment above, I stumbled on another existing > bug in `toString`... I'll add another issue for that one, and follow up with > a separate PR to address, unless you think it makes sense to pack it all into > this PR. > > What do you think: follow up with PRs, or get them into this one? (I already > added the three issues, FYI) I think a follow-up for the existing bug in `toString()` is also reasonable. https://github.com/llvm/llvm-project/pull/218471 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
