Page MenuHomePhabricator

Fix false positives in match()
ClosedPublic

Authored by ardunster on Nov 21 2020, 12:13 PM.

Details

Summary

Static match working as well as member match

Static match correctly returning false on too long input

Separated logic for converting model into array

All tests passing with new version of match()

Merge after committing D365

Test Plan

included goldilocks tests

Diff Detail

Repository
rS SIMPLEXpress
Lint
Lint Not Applicable

Event Timeline

Build is green

Building SimpleExpress on mpm-bionic-000981y1b7c5j as build 37.

See https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/37/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Nov 21 2020, 12:14 PM

Still need to clean up some commented out code and improve the test suite to check for false positives, etc. Also properly arrange and comment the multiple overloads of match() :D

  • Improve tests. Fixed multiple+optional logic and checking for strings longer than models
ardunster retitled this revision from T1406: Fix false positives in match() to Fix false positives in match().Nov 23 2020, 1:18 PM

Build is green

Building SimpleExpress on mpm-bionic-000ayod88z1yf as build 38.

See https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/38/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Nov 23 2020, 1:19 PM
  • Fix commenting and minor errors, improve test tB0014

Build is green

Building SimpleExpress on mpm-bionic-000aywtr10bp6 as build 39.

See https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/39/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Nov 23 2020, 1:29 PM
  • Remove duplicate logic in check_model()

Build is green

Building SimpleExpress on mpm-bionic-000az2y9pgnyn as build 40.

See https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/40/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Nov 23 2020, 1:37 PM

Other than the braces rest looks fine. If it's the standard then I guess it's done.

simplexpress-source/src/unit.cpp
177

Sorry not too sure about this but should this have braces if we are using OTBS? some if else statements are missing braces.

simplexpress-source/src/unit.cpp
177

He's right. We always include the braces, even when it's a one-line-only suite. It prevents errors that way.

205–224

Same here. clang-format should help with this.

simplexpress-source/src/unit.cpp
177

Got it, will tidy those up. The logic was all copy pasted so I wasn't thinking about format yet. I don't actually have clang-format set up yet, never got an answer to my questions about it.

simplexpress-source/src/unit.cpp
177

Eep! You might need to remind me of where the Ponder question is...or put it on Ponder if it isn't there. Hard for me to keep track of questions otherwise.

  • Fix bracketing issues in check_model()
  • Switch Simplex model to FlexArray

Build has FAILED

Building SimpleExpress on mpm-bionic-000f4oe8vcluu as build 41.

Link to build: https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/41/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/41/console

Restricted Application failed remote builds in Restricted Buildable!Nov 28 2020, 10:51 AM

@jcmcdonald This is failing build because it's using the old version of FlexArray, not your current update. Should I change anything or just retry once D362 goes through?

I'm pushing D362 now. Stand by. I'll trigger the rebuild on this.

Build has FAILED

Building SimpleExpress on mpm-bionic-000f4sgq76k41 as build 42.

Link to build: https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/42/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/42/console

Build is green

Building SimpleExpress on mpm-bionic-000f4t59bmj82 as build 43.

See https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/43/ for more details.

I see it wasn't behaving itself the first time, lol. I did the update on the brackets too.

I see it wasn't behaving itself the first time, lol. I did the update on the brackets too.

That was my fault. I forgot that Jenkins runs builds concurrently, rather than consecutively, so I had a bit of a race condition. PawLIB's central build wasn't done before the build for this restarted. Oops. 😅

Well, that makes sense, I guess. :) For some reason it still has the red circle with an X in the build status at the top of the page, though.

Oh, I can fix that by restarting the test via Harbormaster, instead of directly in Jenkins...

Build is green

Building SimpleExpress on mpm-bionic-000f6i63mq7hs as build 44.

See https://jenkins.mousepawmedia.net:8449/job/simplexpress_build/44/ for more details.

Oh, meh, nope. That red circle is linked to the build executed when the Differential is updated. Weird.

It'll resolve on the next update to the diff.

Was there anything else I missed before this can be approved? I fixed the brackets @mahussain pointed out.

This revision is now accepted and ready to land.Nov 30 2020, 6:06 PM
This revision was landed with ongoing or failed builds.Dec 2 2020, 10:51 AM
This revision was automatically updated to reflect the committed changes.