Page MenuHomePhabricator
Differential D361

Fix false positives in match()
ClosedPublic
Actions

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

Details

Reviewers
jcmcdonald
memateo
wdede
mahussain
Maniphest Tasks
T1406: SIMPLEXpress: Fix false positives in Simplex.match()
Commits
rS95f245cfbf8f: Fix false positives in match()
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

ardunster created this revision.Nov 21 2020, 12:13 PM
jenkins added a comment.Nov 21 2020, 12:14 PM

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
ardunster requested review of this revision.Nov 21 2020, 12:14 PM
ardunster planned changes to this revision.Nov 21 2020, 12:16 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

ardunster updated this revision to Diff 1670.Nov 23 2020, 1:17 PM
  • 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
ardunster added reviewers: jcmcdonald, memateo, wdede, mahussain.
jenkins added a comment.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
ardunster updated this revision to Diff 1671.Nov 23 2020, 1:28 PM
  • Fix commenting and minor errors, improve test tB0014
jenkins added a comment.Nov 23 2020, 1:29 PM

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
ardunster updated this revision to Diff 1672.Nov 23 2020, 1:36 PM
  • Remove duplicate logic in check_model()
jenkins added a comment.Nov 23 2020, 1:37 PM

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
mahussain added a comment.Nov 27 2020, 3:09 AM

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.

jcmcdonald added inline comments.Nov 27 2020, 10:40 AM
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.

ardunster added inline comments.Nov 27 2020, 4:42 PM
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.

jcmcdonald added inline comments.Nov 27 2020, 10:26 PM
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.

ardunster updated this revision to Diff 1674.Nov 28 2020, 10:50 AM
  • Fix bracketing issues in check_model()
  • Switch Simplex model to FlexArray
jenkins added a comment.Nov 28 2020, 10:51 AM

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
ardunster marked 4 inline comments as done.Nov 28 2020, 10:53 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?

jcmcdonald added a comment.Nov 28 2020, 10:54 AM

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

jenkins added a comment.Nov 28 2020, 10:56 AM

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

jenkins added a comment.Nov 28 2020, 10:57 AM

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.

jcmcdonald added a comment.Nov 28 2020, 10:57 AM

Aaaand, there you are!

ardunster added a comment.Nov 28 2020, 10:58 AM

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

jcmcdonald added a comment.Nov 28 2020, 10:59 AM
In D361#12449, @ardunster wrote:

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. 😅

ardunster added a comment.Nov 28 2020, 11:00 AM

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.

jcmcdonald awarded a token.Nov 28 2020, 11:39 AM
jcmcdonald added a comment.Nov 28 2020, 12:16 PM

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

jenkins added a comment.Nov 28 2020, 12:17 PM

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.

jcmcdonald added a comment.Nov 28 2020, 12:17 PM

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.

ardunster added a comment.Nov 30 2020, 5:55 PM

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

jcmcdonald accepted this revision.Nov 30 2020, 6:06 PM

Looks good!

This revision is now accepted and ready to land.Nov 30 2020, 6:06 PM
memateo awarded a token.Nov 30 2020, 6:07 PM
This revision was landed with ongoing or failed builds.Dec 2 2020, 10:51 AM
Closed by commit rS95f245cfbf8f: Fix false positives in match() (authored by ardunster). · Explain Why
This revision was automatically updated to reflect the committed changes.
ardunster added a commit: rS95f245cfbf8f: Fix false positives in match().
ardunster mentioned this in D365: Resolving the "Greedy" problem.Dec 12 2020, 5:18 PM

Revision Contents
Changeset List

PathSize
.vscode/
6 lines
simplexpress-source/
include/
simplexpress/
103 lines
60 lines
22 lines
src/
1 line
78 lines
65 lines

Diff 1681

View Options

.vscode/settings.json

Loading...
View Options

simplexpress-source/include/simplexpress/basic_test.hpp

Loading...
View Options

simplexpress-source/include/simplexpress/simplex.hpp

Loading...
View Options

simplexpress-source/include/simplexpress/unit.hpp

Loading...
View Options

simplexpress-source/src/basic_test.cpp

Loading...
View Options

simplexpress-source/src/simplex.cpp

Loading...
View Options

simplexpress-source/src/unit.cpp

Loading...
Copyright (C) 2019 MousePaw Media. All Rights Reserved. · Community Rules · Terms of Use · Privacy Policy · Terms of Development