Skip to content

C++: Sort through the leap year and japanese era queries#2542

Merged
jbj merged 5 commits intogithub:masterfrom
geoffw0:datetime
Dec 23, 2019
Merged

C++: Sort through the leap year and japanese era queries#2542
jbj merged 5 commits intogithub:masterfrom
geoffw0:datetime

Conversation

@geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Dec 17, 2019

I've reviewed the results of the leap year + japanese era dates queries, which are currently run but not displayed by default on LGTM. I've also done a bit of cleanup and tagging.

Adding365DaysPerYear.ql (https://lgtm.com/rules/1509196026030/alerts/)

  • some good results
  • some of the results don't even seem to contain the number 365; this query either needs path explanations so we can see what's going on, or (more likely?) restricting to local dataflow.

I think we can make the results better, but it will require some work/iteration (which is probably not a priority). Should we drop it off LGTM or leave it running/hidden with the intent to come back to this in February perhaps?

UncheckedLeapYearAfterYearModification.ql (https://lgtm.com/rules/1509209928660/alerts/)

  • many of the results of this query look good at a glance
  • false positive where the year is being encoded differently, rather than modified
  • false positive where the month and day are set immediately afterwards, rather than checking
    for leap year
  • adding 100 is common (when the century is being assumed); I'm not sure whether or not these cases are problematic

We can definitely make the results better, but I think there will always be quite a number of false positives for this one. I'm inclined to drop it off LGTM rather than leaving it running/hidden for the forseeable future but I'd like a second opinion.

UncheckedReturnValueForTimeFunctions.ql (https://lgtm.com/rules/1509204275387/alerts/)

  • only three results, but I think they're good
  • I think all of the functions this checks are Windows specific, so perhaps we shouldn't expect to see huge numbers of results for it on LGTM

What do we think about promoting this from warning-medium to warning-high, so that it's displayed by default?

UnsafeArrayForDaysOfYear.ql (https://lgtm.com/rules/1509186658915/alerts/)

  • many of these results are not reported at a sensible/useful location
  • some of the results appear to be coincidental?
  • very disappointing

Demoted from warning-medium to warning-low; will no longer be computed for LGTM

JapaneseEraDate.ql (https://lgtm.com/rules/1510017876327/alerts/)

This was already demoted from warning-medium to warning-low recently in #2528, because there are only three results and they're all bad.

@geoffw0 geoffw0 added the C++ label Dec 17, 2019
@geoffw0 geoffw0 requested a review from a team as a code owner December 17, 2019 10:40
Copy link
Contributor

@jbj jbj 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 the well-written analysis. I've added to our team meeting agenda that we should figure out how to preserve texts like that so they don't get lost.

@jbj jbj merged commit 7e84453 into github:master Dec 23, 2019
@jbj
Copy link
Contributor

jbj commented Dec 23, 2019

If Adding365DaysPerYear.ql is not dragging down performance of the suite, then I suggest leaving it in the suite until February, and at that point we can decide if it's a priority to improve it.

Promoting UncheckedReturnValueForTimeFunctions.ql to warning-high sounds good to me.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants