Skip to content

C++: Mishandling Japanese Era and Leap Year in calculations #1354

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 18, 2019
Merged

C++: Mishandling Japanese Era and Leap Year in calculations #1354

merged 6 commits into from
Jun 18, 2019

Conversation

denislevin
Copy link
Contributor

These queries check for various scenarios where the dates may be mishandled.

  • Calculations not taking into account possible leap year
  • Japanese dates assuming and hard-coding current (or now previous) era Heisei

@denislevin denislevin requested a review from a team as a code owner May 21, 2019 21:51
@jbj jbj added the C++ label May 21, 2019
geoffw0
geoffw0 reviewed May 22, 2019
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Hi, this PR looks really valuable. I've suggested quite a few minor changes, let me know if you'd prefer us to make them instead of you.

I've still to look at the tests and try these queries out a bit.

cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYearGood.c
bool isLeapYear = year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
int *items = new int[isLeapYear ? 366 : 365];
// ...
delete[] items;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the best solution to recommend, or should we suggest simply increasing the array size to 366 all the time?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no silver bullet for this pattern. I am afraid that recommending to always use 366-item arrays may hide the fact that the index offset may still be incorrect. The index offset value must also be set properly for leap years, so devs using this pattern should always be aware of leap years to calculate the offset value correctly. We could add a comment to make this clear.

geoffw0
geoffw0 reviewed May 22, 2019
View reviewed changes
cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qhelp Outdated
<li>U.S. Naval Observatory Website - <a href="https://aa.usno.navy.mil/faq/docs/calendars.php"> Introduction to Calendars</a></li>
<li>Wikipedia - <a href="https://en.wikipedia.org/wiki/Leap_year_bug"> Leap year bug</a> </li>
<li>Microsoft Azure blog - <a href="https://azure.microsoft.com/en-us/blog/is-your-code-ready-for-the-leap-year/"> Is your code ready for the leap year?</a> </li>
</references>
Copy link
Contributor

Choose a reason for hiding this comment

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

The qhelp-preview job failed with [ERROR] ql/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qhelp:16:1: XML document structures must start and end within the same entity.. I believe it wants you to add a </qhelp> tag here.

Copy link
Contributor

Choose a reason for hiding this comment

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

How can we test the qhelp files to avoid these errors in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally tend to just rely on the qhelp-preview job on the pull request, but I can see that an offline tool might be helpful if you're writing a lot of help.

@semmledocs-ac what do you recommend?

@rdmarsh2
Copy link
Contributor

Can you run the autoformatter on the QL files?

semmledocs-ac
semmledocs-ac reviewed May 22, 2019
View reviewed changes
Copy link
Contributor

@semmledocs-ac semmledocs-ac left a comment

Choose a reason for hiding this comment

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

Thanks for submitting these. Very nice accompanying QHelp. I've done an editorial check of the documentation and raised some comments - all small stuff. As per Geoffrey's comment, let us know if you'd prefer us to make these changes instead of you.

semmledocs-ac
semmledocs-ac previously approved these changes Jun 14, 2019
View reviewed changes
Copy link
Contributor

@semmledocs-ac semmledocs-ac left a comment

Choose a reason for hiding this comment

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

Denis, thanks for making those changes.
This all LGTM now from my point of view.

@geoffw0
Copy link
Contributor

geoffw0 commented Jun 14, 2019

Changes LGTM as well. All of the changes I asked for have been made as well. I do have an unanswered question above.

geoffw0
geoffw0 previously approved these changes Jun 14, 2019
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

I think this is about ready to merge and we're keen to do so. We'll probably make a few more refinements to it in the coming days.

Thank you for your contribution!

@geoffw0 geoffw0 mentioned this pull request Jun 14, 2019
Merged
@geoffw0
Copy link
Contributor

geoffw0 commented Jun 14, 2019

There were a couple of pull request check failures due to issues in the qhelp. Here is my proposed solution: denislevin#3

@denislevin
Merge pull request #3 from geoffw0/dates-fix
6a05c84 
CPP: Fixes for C++: Mishandling Japanese Era and Leap Year in calculations github#1354
@denislevin denislevin dismissed stale reviews from geoffw0 and semmledocs-ac via 6a05c84 June 14, 2019 18:21
@denislevin
Copy link
Contributor Author

Please feel free to merge once it's ready! Thank you!

geoffw0
geoffw0 approved these changes Jun 18, 2019
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Let's not delay any further and merge this. There are still a couple of qhelp errors (my fault I think), which I will fix in a follow-up PR.

@geoffw0 geoffw0 merged commit 83ec5f1 into github:master Jun 18, 2019
@geoffw0 geoffw0 mentioned this pull request Jul 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoffw0 geoffw0 geoffw0 approved these changes

+2 more reviewers

@raulgarciamsft raulgarciamsft raulgarciamsft left review comments

@semmledocs-ac semmledocs-ac semmledocs-ac left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@rdmarsh2 rdmarsh2

Labels
C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@denislevin @rdmarsh2 @geoffw0 @raulgarciamsft @semmledocs-ac @jbj