Remove questionable uses of LiteralString class #5685
Conversation
| @@ -3246,7 +3247,7 @@ void __cdecl _alloca_probe_16() | |||
| if (proxy) | |||
| { | |||
| ParseableFunctionInfo * func = proxy->EnsureDeserialized(); | |||
| return LiteralString::NewCopySz(func->GetDisplayName(), scriptContext); | |||
| return JavascriptString::NewCopySz(func->GetDisplayName(), scriptContext); | |||
There was a problem hiding this comment.
@boingoing It seems like PFI::GetDisplayName sometimes returns a non-recycler-allocated buffer, so we can't use NewWithSz here. Is that accurate?
There was a problem hiding this comment.
If you want to get super fancy, you could call GetDisplayNameIsRecyclerAllocated to find out
In reply to: 216497636 [](ancestors = 216497636)
There was a problem hiding this comment.
True. I will do that instead.
There was a problem hiding this comment.
Yes, it is possible the display name is not recycler-allocated. If it's allocated in the bytecode buffer, for example.
|
Surely someone at some time has written a lint rule saying "don't call |
| @@ -3246,7 +3247,7 @@ void __cdecl _alloca_probe_16() | |||
| if (proxy) | |||
| { | |||
| ParseableFunctionInfo * func = proxy->EnsureDeserialized(); | |||
| return LiteralString::NewCopySz(func->GetDisplayName(), scriptContext); | |||
| return JavascriptString::NewCopySz(func->GetDisplayName(), scriptContext); | |||
There was a problem hiding this comment.
Yes, it is possible the display name is not recycler-allocated. If it's allocated in the bytecode buffer, for example.
jackhorton
force-pushed
the
literal-strings
branch
from
660b359 to
6cf039d
Compare
…lass Merge pull request #5685 from jackhorton:literal-strings Found this as a driveby while making a different change. To me, `LiteralString::NewCopy*` is an anti-pattern -- LiteralString doesn't override NewCopy*, so it calls the JavascriptString version regardless, and it gives the impression that some optimization was in mind but not achieved. Additionally, by searching for more cases, I found a number of cases where we were wastefully creating JavascriptStrings for various string constants (most were especially brutal since they were going in rarely used ConcatStrings). Not sure if there is value in actually hardening LiteralString against incorrect uses, but I can implement that if others see value.
Found this as a driveby while making a different change. To me,
LiteralString::NewCopy*is an anti-pattern -- LiteralString doesn't override NewCopy*, so it calls the JavascriptString version regardless, and it gives the impression that some optimization was in mind but not achieved. Additionally, by searching for more cases, I found a number of cases where we were wastefully creating JavascriptStrings for various string constants (most were especially brutal since they were going in rarely used ConcatStrings).Not sure if there is value in actually hardening LiteralString against incorrect uses, but I can implement that if others see value.