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

Move nlsdownlevel.h into winapinls.cpp (#188) #220

Merged
merged 1 commit into from
Oct 27, 2019

Conversation

chris0x44
Copy link
Contributor

Description

Checklist

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

  • I understand README.md. I also understand that acceptance of
    community PRs will be delayed until the test and CI systems are online.
  • 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 CI is online, 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 and
    the C++ Working Draft as a reference (and any other cited standards).
    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.

@chris0x44 chris0x44 requested a review from a team as a code owner October 25, 2019 17:04
stl/src/winapinls.cpp Outdated Show resolved Hide resolved
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Looks good - this is almost [NFC], so it shouldn't require additional test coverage.

(Stephan adds: "NFC" means No Functional Changes.)

stl/src/winapinls.cpp Outdated Show resolved Hide resolved
stl/src/winapinls.cpp Outdated Show resolved Hide resolved
stl/src/winapinls.cpp Outdated Show resolved Hide resolved
stl/src/winapinls.cpp Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

As Casey predicted, Azure Pipelines detected that clang-format needs to be run on this file.

@StephanTLavavej
Copy link
Member

By the way, thanks for working on this!

Note to self: after this is revised and merged on GitHub, we will need to make a Microsoft-internal change to remove nlsdownlevel.h from "setup authoring" (which installs the file as part of Visual Studio). The setup authoring files that need to be changed are for the entire MSVC toolset, which is why they can't be part of this GitHub repo.

@StephanTLavavej
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej
Copy link
Member

I've made those Microsoft-internal changes and I'm running this through our build system.

@StephanTLavavej StephanTLavavej merged commit 99b75ab into microsoft:master Oct 27, 2019
@StephanTLavavej
Copy link
Member

Thanks for the improvement, and congratulations on your first commit to the STL!

Now that you've made the tables constexpr, @CaseyCarter suggested a further improvement that will be possible in the future, which I've filed as #224.

@chris0x44 chris0x44 deleted the remove-nlsdownlevel branch October 28, 2019 18:53
@chris0x44
Copy link
Contributor Author

Thanks :) I'll keep an eye on #224

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

Successfully merging this pull request may close these issues.

5 participants