Skip to content

Improve build instructions and cleanup code - #764

Merged
Stephan T. Lavavej (StephanTLavavej) merged 14 commits into
microsoft:masterfrom
StephanTLavavej:cleanups
Apr 30, 2020
Merged

Improve build instructions and cleanup code#764
Stephan T. Lavavej (StephanTLavavej) merged 14 commits into
microsoft:masterfrom
StephanTLavavej:cleanups

Conversation

@StephanTLavavej

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) commented Apr 29, 2020

Copy link
Copy Markdown
Member
  • Change build.x64 to out\build\x64, fixing README.md: How To Consume inconsistent with How To Build With A Native Tools Command Prompt #518.
    • This significantly improves usability as .gitignore lists /out/.
  • Add "build" to skipped_directories in validate.cpp.
  • Consistently test #if !_HAS_EXCEPTIONS.
    • This still isn't a supported mode.
  • _RAISE was never used for real exceptions.
  • Change unsigned to unsigned int.
    • There were a few cases (outside of LLVM-derived tests) where we weren't following our usual convention. I'm cleaning them up, even within tr1.
    • Additionally, change a couple of C casts to static_casts, slowly working towards src: Change C casts to C++ casts #184.
  • Use {} for make_index_sequence and index_sequence_for.
  • cthread.cpp: Simplify a lengthy conditional operator.
  • Extract __crtLCMapStringA calls and use C++ casts.
    • xstrxfrm.cpp: dstlen can be const.
    • Remove unnecessary parentheses (equality has well-known higher precedence than logical AND).
  • Change ploc == 0 to ploc == nullptr.
  • dllmain.cpp: [[maybe_unused]], preprocessor comments.
  • Use #ifdef _CRT_APP for simplicity.
  • Avoid _underscore_lowercase parameter names.
  • Extract more assignments before comparisons.
    • Also scope size_t i to a for-loop and use preincrement.
    • Immediately initialize ans.
  • More C++ casts.

This significantly improves usability as `.gitignore` lists `/out/`.
This essentially replicates the information in `.gitignore`,
where `/build/` was added by microsoft#581.
This still isn't a supported mode.
There were a few cases (outside of LLVM-derived tests) where we weren't
following our usual convention. I'm cleaning them up, even within tr1.

Additionally, change a couple of C casts to static_casts,
slowly working towards microsoft#184.
In new code, we prefer using `{}` to construct tags,
because `()` resembles a function call. See microsoft#468.
xstrxfrm.cpp: dstlen can be const.

Remove unnecessary parentheses (equality has well-known higher
precedence than logical AND).
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added documentation Related to documentation or comments enhancement Something can be improved labels Apr 29, 2020
@BillyONeal

Copy link
Copy Markdown
Member

Change unsigned to unsigned int

I will high 5 James but not my call :P

@StephanTLavavej

Copy link
Copy Markdown
Member Author

The most important principle is consistency. If enough maintainers want to change our convention to say unsigned instead of unsigned int, then let's do that - but we shouldn't randomly switch between the two.

(It's already somewhat problematic that test code sometimes uses typename instead of class - that was me enjoying my personal preference in test code despite the history of product code - and that some tests use right const when product code uses left const. Now that we're accepting contributions from the community, global consistency is somewhat more important than before, I think.)

Comment thread stl/src/dllmain.cpp Outdated
Comment thread stl/src/xstrxfrm.cpp Outdated
Comment thread stl/src/xstrxfrm.cpp Outdated
@CaseyCarter

Casey Carter (CaseyCarter) commented Apr 29, 2020

Copy link
Copy Markdown
Contributor

(It's already somewhat problematic that test code sometimes uses typename instead of class - that was me enjoying my personal preference in test code despite the history of product code - and that some tests use right const when product code uses left const.

You misspelled "const left" here.

Now that we're accepting contributions from the community, global consistency is somewhat more important than before, I think.)

As possibly the greatest offender / proponent of east const on the team, I agree. When there were fewer maintainers it made sense for a feature's "owner" to write and maintain the attendant tests in a slightly more personalized style. As the number of maintainers grows, this practice is becoming less defensible. I don't mind forcing the Internet to contribute code in our conventional style - that's the barrier for entry to any open-source project - but asking people to learn potentially different sets of undocumented conventions for each test they touch seems like a waste of the time and goodwill of our contributors.

EDIT: That said, I will never write short int or long long int and you can't make me!

@StephanTLavavej

Copy link
Copy Markdown
Member Author

Strongly agreed 😸

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Related to documentation or comments enhancement Something can be improved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

README.md: How To Consume inconsistent with How To Build With A Native Tools Command Prompt

5 participants