This repository has been archived by the owner on Oct 15, 2020. It is now read-only.
chakrashim: fix console autocomplete in VS Code #356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MSLaguana
reviewed
Aug 3, 2017
| @@ -57,18 +56,15 @@ class JavaScriptCallFrame { | |||
| v8::Local<v8::Object> details() const; | |||
|
|
|||
| v8::MaybeLocal<v8::Value> evaluate(v8::Local<v8::Value> expression, | |||
| bool* isError); | |||
| bool returnByValue, bool* isError); | |||
There was a problem hiding this comment.
Should each argument go on its own line, like the other functions around here?
There was a problem hiding this comment.
This seems to be the node convention, the other method has them all on separate lines by coincidence in this case. There's not enough room on the line to support any two of those parameters.
kfarnung
commented
Aug 3, 2017
| @@ -2897,7 +2897,7 @@ bool PersistentBase<T>::IsNearDeath() const { | |||
|
|
|||
| template <class T> | |||
| bool PersistentBase<T>::IsWeak() const { | |||
| return static_cast<bool>(_weakWrapper); | |||
| return _weakWrapper != nullptr; | |||
There was a problem hiding this comment.
This was the cause of the C4800 warning. Apparently VS2017 no longer issues a warning for it, but it seems weird to static cast when we're essentially just checking whether this is null?
kfarnung
force-pushed
the
inspector
branch
2 times, most recently
from
0d82217
to
178eadf
Compare
|
CI looks clean. |
|
One more CI for the rebase: https://ci.nodejs.org/job/chakracore-test-pull-request/135/ |
MSLaguana
approved these changes
Aug 3, 2017
The autocomplete for VS Code (and probably Chrome) sends a blob of JavaScript to the engine which enumerates/filters the properties and returns them. The code wasn't respecting the `returnByValue` parameter and VS Code was unable to get the length of the undefined `value` field in the response. While I was in there I also removed some unneeded fields from the `JavaScriptCallFrame` class and fixed a build warning (C4800) in v8.h. PR-URL: nodejs#356 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
kfarnung
added a commit
that referenced
this pull request
Aug 10, 2017
The autocomplete for VS Code (and probably Chrome) sends a blob of JavaScript to the engine which enumerates/filters the properties and returns them. The code wasn't respecting the `returnByValue` parameter and VS Code was unable to get the length of the undefined `value` field in the response. While I was in there I also removed some unneeded fields from the `JavaScriptCallFrame` class and fixed a build warning (C4800) in v8.h. PR-URL: #356 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The autocomplete for VS Code (and probably Chrome) sends a blob of
JavaScript to the engine which enumerates/filters the properties and
returns them. The code wasn't respecting the
returnByValueparameterand VS Code was unable to get the length of the undefined
valuefieldin the response.
While I was in there I also removed some unneeded fields from the
JavaScriptCallFrameclass and fixed a build warning (C4800) in v8.h.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
chakrashim