Skip to content

Conversation

@SunnyWar
Copy link
Contributor

@SunnyWar SunnyWar commented Sep 1, 2020

These were found with clang.

@SunnyWar SunnyWar requested a review from a team as a code owner September 1, 2020 05:40
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Sep 1, 2020
if (GetFileAttributesExW(_Fname, GetFileExInfoStandard, &_Data)) {
// get file type and return permissions
if (_Pmode != 0) {
if (_Pmode != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use conversion to bool here, aka if (_Pmode)?

Would apply throughout

Copy link
Member

Choose a reason for hiding this comment

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

Our codebase is currently inconsistent about testing pointers directly (or negated), versus comparing them to nullptr. (Unlike integers, where we strongly prefer to explicitly compare them against zero.) I personally like directly testing pointers, because I believe it reads naturally ("if we have pointer, do cool things with it") and avoids the double negation inherent in "not null". However, I think that if we want to consistently test pointers directly, we should do so as a separate cleanup change.

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.

Looks good, thanks! I think this will be ready to check in after making clang-format happy, and changing the _CRT_APP codepath for symmetry (this is not currently built in the GitHub repo but will be in the future).

@CaseyCarter CaseyCarter changed the title Replace 0 with nullptr where appropriate. Replace 0 with nullptr where appropriate Sep 4, 2020
@CaseyCarter CaseyCarter self-assigned this Sep 4, 2020
@CaseyCarter CaseyCarter merged commit 154f9a8 into microsoft:master Sep 4, 2020
@CaseyCarter
Copy link
Contributor

Thanks for the cleanup!

@CaseyCarter CaseyCarter removed their assignment Sep 5, 2020
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.

4 participants