Why store these as fields on the UrlDecoder? Seems like they could just be a single localneedsDecoding, and then instead of doingreturn helper.GetString();, you'd doreturn needsDecoding ? helper.GetString() : value;.

I agree with moving them out of UrlDecoder.
I would like to keep the fieldscontainsUnsafe andcontainsSpaces as separate variables, as I'm working on a PR that's gonna heavily optimizeUrlEncode andUrlDecode for strings that only need space encoding/decoding. Is that OK?

I'd prefer it if you kept them as a single local now. If a subsequent change needs them separated, then that change can do it. Otherwise, if for example that change never materializes or gets merged, we're left with debt.

Sure thing!

Is this really useful? Empty string is already special cased to avoid decoding. So isn't this only valuable in the case where all of the inputs are bytes rather than chars, and to enable that you're then doing an extra null check branch on every AddChar? And even then all of the bytes except for those at the end will end up getting dumped into the char[], forcing it to be allocated. Doesn't seem like a good tradeoff.

Well I found that this halved allocations for theUrlDecode("noescaping") case, as it seems that ASCII chars are added to the decoder asAddByte not asAddChar. I found this behaviour weird, but lazilly instantiating_charBuffer does help in the no escaping case.

Just a follow up: its due tothis line of code.

I don't really understand why "// 7 bit have to go as bytes because of Unicode" but hey

I see, ok. (Seems strange then that we're lazily allocating the byte[] array; doing so suggests we don't expect any ASCII chars in common input, but let's not change it.)

understood - obviously I'm not planning to change any of that behaviour in this PR, but do you have a theory as to why we add ascii chars as bytes not chars?

right but they allocate the array only when needed

No. In the code before this PR, the char[] is always allocated (unless the string is empty).

right. looking more, it looks when having ascii characters, it needs to be encoded by the encoder. bu tif the character is not ascii will be stored as it is without encoding. so I believe what they are doing is collecting ascii charcaters in the array, and everytime need to flush it, will run the encoder just once on the array before the flush. so it is just optimizing to not calling the encoder with every ascii character.

so it is just optimizing to not calling the encoder with every ascii character.

but doesn't the implementation check if there are any bytes in the buffer, and if there are none in the buffer it doesn't flush any bytes
So that means that each ascii character wouldn't flush any bytes, only the first in a string of ascii chars

UrlDecoder is collecting the ascii bytes till either AddChar or GetString get called. at that time it will flush the bytes after encoding it. and then will start collecting the new encountered bytes again and so on.
imagine you have string like aaaaaNaaaaNaaaaNaaa
where a is ascii character and N is not ascii, this will have the byte array get filled 4 times and encoded 4 times. if you didn't collect the ascii in byte array as the code is doing, this mean you'll need to call the encoder the number of 'a' characters in the string.

understood, thanks

If it were me, I'd have written this like:

returnneedsDecoding?helper.GetString():value;