Skip to content
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.

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

Add to_buffer(string) and use it when initializing a buffer #1907

Merged
merged 5 commits into from
Aug 20, 2023

Conversation

Veracity0
Copy link
Contributor

I've wanted to be able to do this for a long time:

buffer b = "Initial contents";

Now I can.

I also moved creation of the (buffer) return value of visit_url() to the top of the method, rather than duplicating the creation in two places.

@Veracity0 Veracity0 requested a review from a team as a code owner August 19, 2023 14:01
@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #1907 (d8cf23b) into main (68b9dca) will increase coverage by 0.00%.
The diff coverage is 82.85%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1907   +/-   ##
=========================================
  Coverage     36.37%   36.38%           
- Complexity    18830    18833    +3     
=========================================
  Files          1083     1083           
  Lines        166605   166619   +14     
  Branches      35391    35393    +2     
=========================================
+ Hits          60609    60622   +13     
+ Misses        96110    96109    -1     
- Partials       9886     9888    +2     
Files Changed Coverage Δ
...et/sourceforge/kolmafia/textui/RuntimeLibrary.java 40.55% <69.23%> (+0.09%) ⬆️
src/net/sourceforge/kolmafia/textui/Parser.java 94.14% <90.90%> (-0.07%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68b9dca...d8cf23b. Read the comment docs.

jaadams5
jaadams5 previously approved these changes Aug 19, 2023
midgleyc
midgleyc previously approved these changes Aug 19, 2023
Copy link
Member

@midgleyc midgleyc 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 good to have in main, but I think there are a fair few scripts (e.g. Ezandora's) which define

buffer to_buffer(string str)
{
  buffer result;
  result.append(str);
  return result;
}

for the convenience, and which will now conflict and error. I think s.replace_string("", "") is also popular.

@Veracity0
Copy link
Contributor Author

Yeah. Ezandora's scripts concern me.
I made an announcement about this, since there will be much lamentation.
Perhaps it will inspire her to upgrade her scripts. :)

On the other hand, we could make the compile error into a warning. If the script is not depending on the RuntimeLibrary version, no big deal if it redefines it - even to something not equivalent.

@Veracity0
Copy link
Contributor Author

Veracity0 commented Aug 19, 2023

And, oh - for replace_string, we don't want a string to auto-coerce into a buffer.

Operator is allowed to check coercion based on context: "assign", "return", "parameter".
But that is checked after auto-coercion - finding a function to do it for you - is done.

Hmm.

@Veracity0 Veracity0 dismissed stale reviews from midgleyc and jaadams5 via 0275b73 August 19, 2023 15:49
@BadHorseMonkey
Copy link
Contributor

I think this is good to have in main, but I think there are a fair few scripts (e.g. Ezandora's) which define
...

Also, your Bastille script. And TourGuide. And those are just the ones I have installed.

@Veracity0
Copy link
Contributor Author

Interesting.

    if (f.overridesLibraryFunction()) {
      String buffer = "Function '" + f.getSignature() + "' overrides a library function.";
      this.warning(f.getLocation(), buffer);
    }

works nicely if I declare the library function like this:

    params = new Type[] {DataTypes.STRING_TYPE};
    functions.add(new LibraryFunction("to_buffer", DataTypes.BUFFER_TYPE, params));

but not if I declare it like this:

    params = new Type[] {DataTypes.STRICT_STRING_TYPE};
    functions.add(new LibraryFunction("to_buffer", DataTypes.BUFFER_TYPE, params));

There is no way for the user to declare a function whose argument is a STRICT_STRING_TYPE

@Veracity0
Copy link
Contributor Author

I made this script:

// The following should generate a warning
buffer to_buffer(string initial)
{
    print("initial = '" + initial + "'");
    buffer buf;
    buf.append(initial);
    return buf;
}

buffer buf1 = to_buffer("Initial content 1");
print("buf1 content = '" + buf1.to_string() + "'");

buffer buf2 = "Initial content 2";
print("buf2 content = '" + buf2.to_string() + "'");

buffer buf3 = $path[Heavy Rains];
print("buf3 content = '" + buf3.to_string() + "'");

which yields:

Function 'to_buffer(string)' overrides a library function. (to_buffer_test.ash, line 2, char 8 to char 33)
initial = 'Initial content 1'
buf1 content = 'Initial content 1'
initial = 'Initial content 2'
buf2 content = 'Initial content 2'
initial = 'Heavy Rains'
buf3 content = 'Heavy Rains'

The good news is that the user defined function really does override the library function.
The bad news is that I didn't REALLY want the $path[] to get coerced into a string.
Which is to say, I'd prefer STRICT_STRING_TYPE.

I don't see an immediate clean solution to the STRING_TYPE != STRICT_STRING_TYPE issue.
Function.paramsMatch:

    while (it1.hasNext() && it2.hasNext()) {
      VariableReference val1 = it1.next();
      VariableReference val2 = it2.next();
      Type paramType1 = base ? val1.getType() : val1.getRawType();
      Type paramType2 = base ? val2.getType() : val2.getRawType();
      if (!paramType1.equals(paramType2)) {
        return false;
      }

It compares Type using equals(). No special casing to pretend STRING_TYPE is the same as STRICT_STRING_TYPE.
Until/unless we rethink how STRICT_STRING_TYPE is implemented, I have two options:

  1. to_buffer(string) is declared with a STRING_TYPE argument and we get useful override warnings
  2. to_buffer(string) is declared with a STRICT_STRING_TYPE argument and we get no warning.

I'm inclined to do #1, for now.

@Veracity0 Veracity0 merged commit db3e4c2 into main Aug 20, 2023
8 checks passed
@Veracity0 Veracity0 deleted the buffer-auto-coercion branch August 20, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants