Page MenuHomePhabricator

Pawlib plus operator
ClosedPublic

Authored by memateo on Oct 2 2020, 9:05 AM.

Details

Reviewers
jcmcdonald
wdede
Maniphest Tasks
T1314: Onestring: operator+()
Commits
rP31794db06218: Pawlib plus operator
Required Signatures
Restricted Legalpad Document
Summary

Added the plus operator, accepting types std::string, char, onestring, and char*

Test Plan

None yet

Diff Detail

Repository
rP PawLIB
Lint
Lint Not Applicable

Event Timeline

Build is green

Building PawLIB on mpm-bionic-00092yrhzxcty as build 552.
Running test suite P-sB40.

See https://jenkins.mousepawmedia.net:8449/job/pawlib_build/552/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Oct 2 2020, 9:06 AM

Two things you need to do here:

  1. Add me as a reviewer. You can do this by specifying jcmcdonald under reviewers when you created the Diff, or by editing it here and adding me.
  1. Link this to your Maniphest Task via the Edit Related Objects option to the right of the description of either this or the Task.

Excellent start! I have a few suggestions inline.

Aside from that, there are some non-coding components to this:

  1. CSI Comments. I address that inline.
  1. Testing! Add tests to onestring_tests.hpp and onestring_tests.cpp. See the existing tests to understand how. (These tests are written for Goldilocks 1.0.)
  1. Documentation. Make sure the docs are up-to-date with your changes. I think they are, but you should be in the habit of double-checking. They're in the docs/source directory, and a live copy from the devel branch is always visible at https://mousepawmedia.net/docs/pawlib/onestring/onestring.html.

Get comfortable with P1, also known as the Revision Checklist.

1Before landing, each Revision should...
2
3(1) Have an associated Maniphest Task.
4(2) Accomplish the feature(s) it was designed to accomplish. [In some cases, the feature itself may be dropped, and only bugfixes and/or optimizations landed instead.]
5(3) Have merged/rebased all changes from `devel` into itself, and all conflicts resolved. ($ git pull --rebase origin devel)
6(4) Have binaries and unnecessary cruft untracked and removed. (Keep an eye on .gitignore!)
7(5) Compile and run properly - this should be confirmed via Harbormaster/Jenkins (if available).
8(6) Be free of compiler errors and warnings (must compile with `-Wall -Wextra -Werror`).
9(7) Be Valgrind pure (no memory leaks detected).
10(8) Comply with Coding and Technical standards.
11(9) Be free of linter errors. ($ arc lint --lintall)
12(10) Be fully CSI commented.
13(11) Have an up-to-date build script (generally CMake) if relevant.
14(12) Contain relevant LIT tests, if the project is Goldilocks capable.
15(13) Have a Test Plan, generally containing a list of Goldilocks tests the reviewer should run.
16(14) Be reviewed, built, tested, and approved by at least one trusted reviewer (Staff or Trusted Contributor).
17(15) Have up-to-date Sphinx documentation, which compiles with no warnings.
18(16) Have all reviewer comments processed and marked "Done".

pawlib-source/src/onestring.cpp
990

Why do you call it nostr? The name isn't very clear. Perhaps new_string would be better.

990–992

I believe you can leverage the constructor for onestring to define lhs as the initial value, instead of having a separate append for lhs.

Here's why I use this particular constructor syntax.

998–1000

Same here. You'll need to go through and apply this to the rest of the code.

This revision now requires changes to proceed.Oct 3 2020, 4:43 PM
pawlib-source/src/onestring.cpp
987–988

Each function needs a documentation comment. I know, most of the functions here don't have that, but that needs to be fixed.

  • Added some tests, used ostr constructor, changed variable name

Build is green

Building PawLIB on mpm-bionic-0006yd2yoybnv as build 553.
Running test suite P-sB40.

See https://jenkins.mousepawmedia.net:8449/job/pawlib_build/553/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Oct 25 2020, 1:45 PM

I still need to add documentation, just want some feedback code wise

Everything looks good to me!

pawlib-source/include/pawlib/onestring_tests.hpp
3023

In the tests I wrote in SIMPLExpress, I just assumed it would be numerically the next test in line, unless you got direction otherwise?

memateo added inline comments.
pawlib-source/include/pawlib/onestring_tests.hpp
3023

Ah okay, I thought it was an assigned thing, I'll go back and change this when I register the test in the appropriate file. Thanks!

Looks ok to me! just one quick question though, where are the "PL_ASSERT_*" family defined?? I assume there are somewhere in Pawlib but I cannot find them.

The assertions should be in pawlib-source/include/pawlib/goldilocks_assertions.hpp

I was just reviewing the PawLIB docs in the process of writing SIMPLExpress docs, and I found this info which will be helpful to you in writing an ID for your test.

I'm really pleased with this. I'm approving provisionally on the documentation; if you find you need to update the docs, please do so in this Diff and pitch it back over for review. Otherwise, if the docs are already up-to-date (they might be?), you may land.

This revision is now accepted and ready to land.Nov 3 2020, 10:55 AM
  • Added comments and finalized testing
This revision now requires review to proceed.Nov 4 2020, 5:25 PM

Build is green

Building PawLIB on mpm-bionic-00023vhd2eqg8 as build 554.
Running test suite P-sB40.

See https://jenkins.mousepawmedia.net:8449/job/pawlib_build/554/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Nov 4 2020, 5:27 PM
  • Fixed onestring.cpp plus op duplicates

Build has FAILED

Building PawLIB on mpm-bionic-00023xmqb0a98 as build 555.

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

Restricted Application failed remote builds in Restricted Buildable!Nov 4 2020, 5:29 PM
jcmcdonald added inline comments.
pawlib-source/src/onestring.cpp
988–1044

Those weren't duplicates. Those are the implementations for the functions you declared in the header. The build is now failing:

onestring_tests.cpp:(.text._ZN20TestOnestring_OpPlus3runEv[_ZN20TestOnestring_OpPlus3runEv]+0x42): undefined reference to `operator+(onestring const&, char const&)'
onestring_tests.cpp:(.text._ZN20TestOnestring_OpPlus3runEv[_ZN20TestOnestring_OpPlus3runEv]+0xb5): undefined reference to `operator+(onestring const&, char const*)'
onestring_tests.cpp:(.text._ZN20TestOnestring_OpPlus3runEv[_ZN20TestOnestring_OpPlus3runEv]+0x14e): undefined reference to `operator+(onestring const&, char const*)'
onestring_tests.cpp:(.text._ZN20TestOnestring_OpPlus3runEv[_ZN20TestOnestring_OpPlus3runEv]+0x1d9): undefined reference to `operator+(onestring const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
onestring_tests.cpp:(.text._ZN20TestOnestring_OpPlus3runEv[_ZN20TestOnestring_OpPlus3runEv]+0x272): undefined reference to `operator+(onestring const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
onestring_tests.cpp:(.text._ZN20TestOnestring_OpPlus3runEv[_ZN20TestOnestring_OpPlus3runEv]+0x2f8): undefined reference to `operator+(onestring const&, onestring const&)'
onestring_tests.cpp:(.text._ZN20TestOnestring_OpPlus3runEv[_ZN20TestOnestring_OpPlus3runEv]+0x35b): undefined reference to `operator+(onestring const&, onestring const&)'
pawlib-source/src/onestring_tests.cpp
161–168

You don't want to register the tests with the exact same number each time.

This revision now requires changes to proceed.Nov 4 2020, 5:32 PM

Whoops. Lesson learned: read code before you delete it! Should be able to fix it pretty soon

Build is green

Building PawLIB on mpm-bionic-00024cr54twev as build 556.
Running test suite P-sB40.

See https://jenkins.mousepawmedia.net:8449/job/pawlib_build/556/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Nov 4 2020, 5:49 PM

Great. You should mark that comment as done here on Phabricator.

One more thing...

pawlib-source/src/onestring_tests.cpp
161–167

You don't want to reuse test ID numbers. Try this instead.

This revision now requires changes to proceed.Nov 4 2020, 5:50 PM

Build is green

Building PawLIB on mpm-bionic-00024g76bs2ya as build 557.
Running test suite P-sB40.

See https://jenkins.mousepawmedia.net:8449/job/pawlib_build/557/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Nov 4 2020, 5:53 PM
memateo marked an inline comment as done.
This revision is now accepted and ready to land.Nov 4 2020, 5:54 PM

Awesome! However I am getting a weird error, I posted it on ponder too.

memateo mentioned this in Unknown Object (Ponder Question).Nov 4 2020, 6:05 PM
This revision was automatically updated to reflect the committed changes.
memateo mentioned this in Unknown Object (Ponder Answer).Nov 4 2020, 6:10 PM