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

Fix the usage of uninitialized variable in adaptive_shared_batch_scheduler and windows build error. #42032

Merged
merged 2 commits into from
Aug 17, 2020

Conversation

kitstar
Copy link
Contributor

@kitstar kitstar commented Aug 4, 2020

Same as #41745 #41745
The variable best_score is uninitialized, but the old PR was reverted because it broke Windows Build.
The new PR fixs the build problem.

@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Aug 4, 2020
@gbaned gbaned self-assigned this Aug 4, 2020
@gbaned gbaned added the comp:core issues related to core part of tensorflow label Aug 4, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Aug 4, 2020
@gbaned gbaned requested a review from jaingaurav August 4, 2020 13:04
@gbaned gbaned requested review from jaingaurav and removed request for jaingaurav August 11, 2020 16:50
@@ -425,7 +425,7 @@ void AdaptiveSharedBatchScheduler<TaskType>::MaybeScheduleNextBatch() {
return;
}
auto best_it = batches_.end();
double best_score;
double best_score = (std::numeric_limits<double>::max)();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the parenthesis around max needed? I thought the problem with the old PR was simply the missing parenthesis at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are the parenthesis around max needed? I thought the problem with the old PR was simply the missing parenthesis at the end.

Ref the discussion on MSDN and stackoverflow,

"The <Windows.h> header file that includes macro definitions named max and min:
#define max(a,b) (((a) > (b)) ? (a) : (b))
The preprocessor replaces the max identifier in the expression:
std::numeric_limits<size_t>::max() to std::numeric_limits<size_t>::(((a) > (b)) ? (a) : (b))

Wrap the call to max with parenthesis, which prevents the macro expansion.
" by Piotr Skotnicki

@gbaned
Copy link
Contributor

gbaned commented Aug 13, 2020

@kitstar Can you please check @jaingaurav's comments and keep us posted ? Thanks!

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Aug 16, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 16, 2020
@tensorflow-copybara tensorflow-copybara merged commit 48ee9cd into tensorflow:master Aug 17, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:core issues related to core part of tensorflow ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants