Skip to content

Conversation

@SunnyWar
Copy link
Contributor

@SunnyWar SunnyWar commented Feb 19, 2020

Description

Refactoring of <complex>.

  1. Replace magic expression with constexpr
  2. Remove unused functions
  3. Put most likely to first conditional
  4. Simply negation using abs() where applicable
  5. Minor formatting

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@SunnyWar SunnyWar requested a review from a team as a code owner February 19, 2020 00:55
@msftclas
Copy link

msftclas commented Feb 19, 2020

CLA assistant check
All CLA requirements met.

@StephanTLavavej StephanTLavavej changed the title <complex> simply and cleanup <complex> simplify and cleanup Feb 19, 2020
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Feb 19, 2020
@StephanTLavavej
Copy link
Member

The x64 check is failing due to clang-format validation. Click on the "Details" and the log will show you the diffs that clang-format wants to make (looks like trimming trailing whitespace and aligning consecutive assignments).

We recommend running clang-format before pushing commits to a PR, but you can manually make edits until the checks pass for small changes.

@SunnyWar
Copy link
Contributor Author

We recommend running clang-format before pushing commits to a PR, but you can manually make edits until the checks pass for small changes.

Done. Please let me know if anything else needs to be done.

@barcharcraz
Copy link
Contributor

barcharcraz commented Feb 20, 2020 via email

@SunnyWar
Copy link
Contributor Author

ping

@StephanTLavavej
Copy link
Member

My apologies for the delay in reviewing - I've been busy analyzing the features and LWG issues that were recently voted in at the Prague meeting (and other things, like working with a compiler dev to find the root cause of a back-end crash, that took most of last week).

We're trying to drain the backlog of pull requests, especially the oldest. Your PR appears to be fairly straightforward, but I want to take some time to think about each transformation and whether it preserves correctness. At the moment I have a couple of higher priorities (filing issues for Prague features and reviewing make_shared for arrays) but I will try to review your PR soon; thank you for your patience.

@SunnyWar
Copy link
Contributor Author

SunnyWar commented Mar 3, 2020

Note #367. Either this or it may need changes depending upon which goes in first.

@SunnyWar
Copy link
Contributor Author

#367 is marked cxx20 so it should not be an impediment.

@SunnyWar
Copy link
Contributor Author

SunnyWar commented Apr 2, 2020

What can I do to push this along? My plan was to work on one thing at a time but I didn't realize that meant getting something done only every 2-3 months.

@StephanTLavavej
Copy link
Member

Double apologies - since my last reply, the world exploded and our productivity hasn't been 100%. I managed to digest the output of the Prague meeting, but I'm still working on helping @AdamBucior get #309 in (recently pushed a near-final batch of product code changes). That has been higher priority because it's a C++20 feature, it's an even older PR (since November 🙀 ), and it's blocking our intern @Weheineman from working on make_shared_for_overwrite. Usually it doesn't take me a month to process a single contributed feature. We appreciate your PR and want to get to it, we simply haven't had the capacity to do so.

I think the thing that would help the most would be to rewrite the commit history so instead of recording the evolution of your work, it presents the changes as batches of systematic work - e.g. one commit purely changing things to constexpr, another commit removing unused functions, etc. It is much easier to review systematic changes in isolation (where mistakes or inconsistencies are easier to see) compared to an effectively-squashed diff where everything is changing and maximum scrutiny is necessary to verify that each transformation is correct and desirable.

Also, I was somewhat concerned about your "Put most likely to first conditional" changes - is the assumption that most numbers are positive? We generally don't reorder this kind of control flow (unlike algorithms where early tests can save O(N) operations or something) - perhaps we're trusting optimizers too much, but I'd like to hear more of the rationale here.

It's okay to have multiple PRs in flight affecting different areas, by the way. It makes our backlog larger but don't let that stop you 😸

Finally, we recently got a good chunk of our tests running in GitHub (another high-priority work item that jumped to the front of the queue) - I'll kick off a test run for you. You can also run the tests locally, following the instructions in the README.

@StephanTLavavej
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@SunnyWar
Copy link
Contributor Author

SunnyWar commented Apr 3, 2020

@StephanTLavavej Thank for the detailed comments.

since my last reply, the world exploded and our productivity hasn't been 100%

I understand. It feels like there are not enough resources to adequately address what is needed.

I think the thing that would help the most would be to rewrite the commit history so instead of recording the evolution of your work

Oh darn. I was hoping you wouldn't ask for that. That means I have to go figure out how to get git to do what I want. It's good for me to learn this stuff though so I'm not complaining.

Also, I was somewhat concerned about your "Put most likely to first conditional" changes

I'll have to go through those changes but as I recall some of them do make that assumption because it's probably a valid one to make. For example, for computing log. I also considering adding [[likely]] and [[unlikely]] but I didn't want this to turn into a C++20 PR.

It's okay to have multiple PRs in flight affecting different areas

Another thing about git I have to learn to do.

Finally, I was not aware the tests were ready to run locally. I look forward to trying that out.

@miscco
Copy link
Contributor

miscco commented Apr 3, 2020

Oh darn. I was hoping you wouldn't ask for that. That means I have to go figure out how to get git to do what I want. It's good for me to learn this stuff though so I'm not complaining.

Dont worry too much, that is rather easy.

  1. For the first run I suggest a backup branch git branch backup
  2. Reset your history to your branch point from master git reset --mixed sha_of_branchpoint
  3. Stage the things that belong together and commit them
    ... repeat 2.

@SunnyWar SunnyWar closed this Apr 13, 2020
@SunnyWar SunnyWar deleted the stl_complex branch April 13, 2020 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants