separate ASTVisitor.visit for ExpressionNode #490
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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #490 +/- ##
==========================================
- Coverage 83.49% 83.48% -0.01%
==========================================
Files 11 11
Lines 8483 8484 +1
==========================================
Hits 7083 7083
- Misses 1400 1401 +1
Continue to review full report in Codecov by Sentry.
|
abstract classes should not have visitor functions, but rather only do dynamic dispatch. This becomes more important later with dlang-community#409
WebFreak001
force-pushed
the
explicit-dynamicdispatch
branch
from
edd03fa
to
0ed83f7
Compare
|
✅ PR OK, no changes in deprecations or warnings Total deprecations: 0 Total warnings: 0 Build statistics: ------ libdparse statistics ------
statistics (-before, +after)
-library size=3432236 libdparse.a
+library size=3434196 libdparse.a
rough build time=16s
------ DCD statistics ------
statistics (-before, +after)
client size=1055768 bin/dcd-client
server size=3006064 bin/dcd-server
-rough build time=75s
+rough build time=74s
-DCD run_tests.sh Elapsed (wall clock) time (h:mm:ss or m:ss): 0:05.80
-DCD run_tests.sh Maximum resident set size (kbytes): 12832
+DCD run_tests.sh Elapsed (wall clock) time (h:mm:ss or m:ss): 0:05.82
+DCD run_tests.sh Maximum resident set size (kbytes): 9704
short requests: (215x)
min request time = 0.009ms
- 10th percentile = 0.120ms
- median time = 0.421ms
- 90th percentile = 0.763ms
- max request time = 1.464ms
+ 10th percentile = 0.111ms
+ median time = 0.430ms
+ 90th percentile = 0.717ms
+ max request time = 1.516ms
top 5 GC sources in server:
bytes allocated, allocations, type, function, file:line
7554112 81399 void[] std.array.Appender!(DSymbol*[]).Appender.ensureAddable.__lambda9 /opt/hostedtoolcache/dc/dmd-2.102.2/x64/dmd2/linux/bin64/../../src/phobos/std/array.d:3577
7536640 460 void[] std.array.Appender!(TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;")[]).Appender.ensureAddable.__lambda9 /opt/hostedtoolcache/dc/dmd-2.102.2/x64/dmd2/linux/bin64/../../src/phobos/std/array.d:3577
2277376 278 void[] std.array.Appender!(char[][]).Appender.ensureAddable.__lambda9 /opt/hostedtoolcache/dc/dmd-2.102.2/x64/dmd2/linux/bin64/../../src/phobos/std/array.d:3577
1830176 57193 std.array.Appender!(dsymbol.symbol.DSymbol*[]).Appender.Data std.array.Appender!(DSymbol*[]).Appender.this /opt/hostedtoolcache/dc/dmd-2.102.2/x64/dmd2/linux/bin64/../../src/phobos/std/array.d:3452
- 1508352 604 void[] std.array.Appender!(const(TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;"))[]).Appender.ensureAddable.__lambda9 /opt/hostedtoolcache/dc/dmd-2.102.2/x64/dmd2/linux/bin64/../../src/phobos/std/array.d:3577
+ 1405952 598 void[] std.array.Appender!(const(TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;"))[]).Appender.ensureAddable.__lambda9 /opt/hostedtoolcache/dc/dmd-2.102.2/x64/dmd2/linux/bin64/../../src/phobos/std/array.d:3577Full build output |
|
the "Maximum resident set size" seems unlikely to have changed drastically with this PR (in fact, only stack usage might have slightly increased through the introduced additional function call for compatibility) We probably need to investigate about a better approach for tracking peak memory usage in DCD. Maybe using valgrind massif? |
|
merging with reduced coverage since the missing coverage is in a deprecated function that will be removed eventually. (and is still covered by DCD tests) |
ghost
reviewed
May 8, 2023
| */ | ||
| void dynamicDispatch(const ExpressionNode n) | ||
| { | ||
| switch (typeMap.get(typeid(n), 0)) |
There was a problem hiding this comment.
The following casts are not good as they could. There should be a bit cast based on the case:
case 1: visit(*cast(AddExpression*) &n); break;
/*etc*/
case 48: visit(*cast(XorExpression*) &n); break;
So far there are still dynamic casts, i.e calls to runtime _d_obj_cast, although those calls always succeed.
There was a problem hiding this comment.
I don't think using bitcasts for class inheritance is safe, since the cast manipulates the pointer value, e.g.
Casting a class object to an interface consists of adding the offset of the interface's corresponding vptr to the address of the base of the object. Casting an interface ptr back to the class type it came from involves getting the correct offset to subtract from it from the object.Interface entry at vtbl[0]. Adjustor thunks are created and pointers to them stored in the method entries in the vtbl[] in order to set the this pointer to the start of the object instance corresponding to the implementing method.
There was a problem hiding this comment.
No interface involved here. this is still the same, I mean interpreted as opaque pointers for example, in both cases.
There was a problem hiding this comment.
if it's safe to do a bitcast, I trust the compiler to do it here (possibly only with optimization enabled) instead. I don't want to rely on the ABI internals for this
There was a problem hiding this comment.
The compiler can't do it for you here as obviously the type of the parameter is of a base node. So to conclude, I won't insist more but at least you are aware of the issue now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
abstract classes should not have visitor functions, but rather only do dynamic dispatch. This becomes more important later with #409