Skip to content
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.

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

questions about <xmath.hpp> #3942

Closed
achabense opened this issue Aug 10, 2023 · 4 comments
Closed

questions about <xmath.hpp> #3942

achabense opened this issue Aug 10, 2023 · 4 comments
Labels
question Further information is requested

Comments

@achabense
Copy link
Contributor

achabense commented Aug 10, 2023

Solved
  1. It seems to me that all these functions are just defined, but are neither exported nor called. Can we remove them?

    STL/stl/src/xmath.hpp

    Lines 87 to 96 in ed8150e

    int _Stopfx(const char**, char**) noexcept;
    _In_range_(0, maxsig) int _Stoflt(
    const char*, const char*, char**, _Out_writes_(maxsig) long[], _In_range_(1, 4) int maxsig) noexcept;
    _In_range_(0, maxsig) int _Stoxflt(
    const char*, const char*, char**, _Out_writes_(maxsig) long[], _In_range_(1, 4) int maxsig) noexcept;
    int _WStopfx(const wchar_t**, wchar_t**);
    _In_range_(0, maxsig) int _WStoflt(
    const wchar_t*, const wchar_t*, wchar_t**, _Out_writes_(maxsig) long[], _In_range_(1, 4) int maxsig) noexcept;
    _In_range_(0, maxsig) int _WStoxflt(
    const wchar_t*, const wchar_t*, wchar_t**, _Out_writes_(maxsig) long[], _In_range_(1, 4) int maxsig) noexcept;
  2. It seems these functions(also their _Fxp and _Lxp versions) are neither defined nor referred-to. Can we remove them?

    STL/stl/src/xmath.hpp

    Lines 117 to 125 in ed8150e

    double _Xp_getw(const double*, int) noexcept;
    double* _Xp_setn(double*, int, long) noexcept;
    double* _Xp_setw(double*, int, double) noexcept;
    double* _Xp_addh(double*, int, double) noexcept;
    double* _Xp_mulh(double*, int, double) noexcept;
    double* _Xp_movx(double*, int, const double*) noexcept;
    double* _Xp_addx(double*, int, const double*, int) noexcept;
    double* _Xp_ldexpx(double*, int, int) noexcept;
    double* _Xp_mulx(double*, int, const double*, int, double*) noexcept;
  1. I notice that _Dval, _Fval and _Lval are using wrong array size, and are only used/passed(only by _Dnorm(_Dval*) and _FDnorm(_Fval*)) via pointers. Can we correct array size for them?

    STL/stl/src/xmath.hpp

    Lines 99 to 102 in ed8150e

    union _Dval { // pun floating type as integer array
    unsigned short _Sh[8];
    double _Val;
    };

    STL/stl/src/xmath.hpp

    Lines 128 to 131 in ed8150e

    union _Fval { // pun floating type as integer array
    unsigned short _Sh[8];
    float _Val;
    };

    STL/stl/src/xmath.hpp

    Lines 155 to 158 in ed8150e

    union _Lval { // pun floating type as integer array
    unsigned short _Sh[8];
    long double _Val;
    };
@achabense achabense added the question Further information is requested label Aug 10, 2023
@frederick-vs-ja
Copy link
Contributor

Some functions are actually used via tricky macro expansions, see #3622 (comment).

I attempted to remove some of them but have given up (at least for now) due these annoying (although helpful) macros.

@frederick-vs-ja
Copy link
Contributor

Can we correct array size for them?

Instead of correcting the sizes, I think we can and should eliminate the type punning - by just using something like struct _Dval_v2 { unsigned short _Sh[4]; } and std::bit_cast.

@StephanTLavavej
Copy link
Member

We can't mess with _Dconst, that appears on the export surface (and has a TRANSITION, ABI comment).

I think we might be able to mess with _Fval, _Dval, _Lval based on our clearer understanding of the differences between:

  • Dllexported functions (can't remove them, can't change their interfaces, currently can't add them unless we revisit that policy)
  • Import lib-injected functions (can't remove them, can't change their interfaces, but can add more)
  • Cross-TU functions that aren't dllexported or import lib-injected (can change freely)

I think that these are cross-TU but not dllexported or import lib-injected. We should also be able to rework extern const _Dconst _FEps; etc. (as they themselves are not dllexported).

To be safe, we should _v2 both the types and the functions. Even if mismatches aren't a concern, this makes reasoning about the code much easier.

Please note - messing with this old code is fairly low value and the maintainer team has sharply limited review capacity for the foreseeable future. Simple transformations are probably low risk but I am not especially eager to review large-scale changes in this area at this time. Additionally, I believe that much of this code may simply be unused except for bincompat - e.g. _Stodx was the entry point for most of it, but we've moved on to using an inline _Stodx_v3 that bypasses all of this code and simply calls strtod. Overall I am interested in changes that remove lots of code (e.g. #3936 was great), and changes that improve performance for active code. However, reducing the sizes of data structures used by code-retained-for-bincompat-only is not really making anyone's experience better.

So what I would be looking for is:

  • Changes that remove code, with brief analysis of why this is a compatible change,
  • Changes that improve efficiency (e.g. reducing overly-bloated unions), with a brief explanation of how this codepath is still active for modern users.

@achabense
Copy link
Contributor Author

Thanks for clarification! I'm much more confident about bincompat things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants