Jump to conversation
Unresolved conversations (5)
@thewilsonator thewilsonator Oct 29, 2018
Is this working?
test/compilable/cppmangle.d
Geod24
@ibuclaw ibuclaw Oct 23, 2018
Why was this moved?
src/dmd/cppmangle.d
ibuclaw Geod24
@ibuclaw ibuclaw Oct 23, 2018
I guess this means that we now have mixed C++98 and C++11 support in the mangler, and no way to choose (on the D side) whether we explicitly want one or the other?
test/runnable/cpp_stdlib.d
Geod24
@ibuclaw ibuclaw Oct 22, 2018
Full stop?
src/dmd/cppmangle.d
Geod24
@jacob-carlborg jacob-carlborg Oct 11, 2018
Looks like another reason to add an Optional type to DMD.
src/dmd/cppmangle.d
Resolved conversations (26)
@ibuclaw ibuclaw Oct 24, 2018
As the first line didn't have a period, when reading this I started the parsing the second line as part of the same sentence. Not disputing whether it looks fine in the generated docs, however I won't likely ever read from the web. :-)
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
If only used `bool* = null` instead of an `out bool`... Perhaps `unused` is a better name than `useless` for a local var.
Outdated
src/dmd/cppmangle.d
Geod24
@ibuclaw ibuclaw Oct 23, 2018
and needs an ending `E`. You could also say requires here, need sounds urgent. :-)
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
`its`
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
This (and maybe other places, I haven't looked too deep), could be rephrased to use a little less `we`. For instance (feel free to correct as you see fit): ``` Since semantic hasn't run yet, all that is handled here are `TypeIdentifier`, but we still need to check if it's an expression or not. ```
Outdated
src/dmd/cppmangle.d
Geod24
@ibuclaw ibuclaw Oct 23, 2018
This comment - `TemplateParameter` or `null` - does not match the return type `bool`.
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
Keeping parameters on the same line is more conformant to coding style.
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
What column length are you keeping yourself to here? I would say 80 is standard, anything shorter just looks out of place.
Outdated
src/dmd/cppmangle.d
Geod24
@ibuclaw ibuclaw Oct 23, 2018
I think you mean "`AliasDeclaration`s" as a plural here.
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
semantic passes are destructive. s/loose/lose/ ?
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
once semantic has run ?
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
Comma seems out of place here.
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
Write it as `asType()`, same reasons as `asFuncDecl`.
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
Might as well give this some ddoc love.
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
Maybe briefly explain this in the form of C++ mangle spec, perhaps even in the (same as passing `&function`) comment above.
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
`tiparent` or `parentti`?
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
It would be nice to have `asFuncDecl()` here so that it's clear that it has a side effect. Same goes for other places where you do this.
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 23, 2018
Using `.mangleof` testing is good, however link testing is better - though not a blocker for this.
test/compilable/cppmangle.d
Geod24
@ibuclaw ibuclaw Oct 22, 2018
I don't think set and reset are appropriate names here, one is `setcontext`, and the other `maybe_swapcontext`. Looking at the usage pattern of them though, `push` and `pop` seems a better fit (the Context could be chained together internally, rather than putting the brunt on the caller to keep a reference to `prev`). Not blocking however.
Outdated
src/dmd/cppmangle.d
Geod24
@ibuclaw ibuclaw Oct 22, 2018
DMD style doesn't require space before opening parens.
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 22, 2018
As this is a ddoc comment (and others), shouldn't it be all punctuated?
src/dmd/cppmangle.d
ibuclaw Geod24
@ibuclaw ibuclaw Oct 22, 2018
Least specialized
Outdated
src/dmd/cppmangle.d
@ibuclaw ibuclaw Oct 22, 2018
Probably should have a comment saying why you're using visit instead of accept here.
src/dmd/cppmangle.d
@jacob-carlborg jacob-carlborg Oct 11, 2018
It's a bit misleading to have a `Returns` section for a method that returns `void`.
Outdated
src/dmd/cppmangle.d
Geod24
@jacob-carlborg jacob-carlborg Oct 11, 2018
A bit inconsistent here that none of the other branches have curly braces.
Outdated
src/dmd/cppmangle.d
@jacob-carlborg jacob-carlborg Oct 11, 2018
Can we please avoid abbreviations like this.
Outdated
src/dmd/cppmangle.d
jacob-carlborg Geod24